Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Light refactor of Helm chart install/upgrade component #6069

Open
catherineluse opened this issue May 31, 2022 · 0 comments
Open

Light refactor of Helm chart install/upgrade component #6069

catherineluse opened this issue May 31, 2022 · 0 comments
Labels
kind/tech-debt Technical debt

Comments

@catherineluse
Copy link
Contributor

This issue is a follow-up to #6068. While I was documenting the logic for how Rancher handles Helm chart app values, I saw some opportunities to refactor https://github.com/rancher/dashboard/blob/release-2.6.5/pages/c/_cluster/apps/charts/install.vue.

Here are the three changes I would make:

  • Avoid having two really long functions - fetch and actionInput - where all the logic is happening. It would be easier to read and understand if, within those functions, we had some logic broken out into smaller functions with names like setUpValuesForInstall or setUpValuesForUpgrade.
  • Avoid removing global values just to re-add them again. Currently we have two functions - removeGlobalValuesFrom and addGlobalValuesTo. Each does the opposite. removeGlobalValuesFrom removes each value from global values if it is equal to what is the current and correct value. Then addGlobalValuesTo calls setIfNotSet to re-add each value again. It would be better to use the lodash merge function (which is already used elsewhere in the file) to handle all the global values in actionInput at the very end of the workflow. This would make it harder to introduce some error when we change or add a global value, which we had to do for project monitoring.
  • Convert it to typescript. While documenting the component I had to write out which values were strings, and what keys were in certain returned objects. It would be cleaner if that was in Typescript.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/tech-debt Technical debt
Projects
None yet
Development

No branches or pull requests

2 participants