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

Improve chectl helm installer to fix cheWorkspacesNamespace che-ws-<username> problem #18459

Closed
cccs-eric opened this issue Nov 25, 2020 · 3 comments
Assignees
Labels
area/chectl Issues related to chectl, the CLI of Che kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Milestone

Comments

@cccs-eric
Copy link
Contributor

Is your enhancement related to a problem? Please describe.

I am using chectl with --installer=helm and --helm-patch-yaml=values.yaml and when I put the following in values.yaml

global:
  cheWorkspacesNamespace: "che-ws-<username>"

chectl will crash since it is "flatting" the yaml fields and setting them individually using --set global.cheWorkspacesNamespace=che-ws-<username>. The set argument is not escaped and the shell will interpret < and >. So in order to make it work, I had to single-quote the value in yaml:

global:
  cheWorkspacesNamespace: "'che-ws-<username>'"

which is not a good user experience.

Describe the solution you'd like

Here is what we have in chectl:

let command = `helm upgrade --install che --force --namespace ${flags.chenamespace} ${setOptions.join(' ')} ${multiUserFlag} ${tlsFlag} ${destDir}`;

we do a join on the options set. What I would like to see is instead of using multiple --set switches, use -f values.yaml and you don't have to parse values.yaml and flat it. If you use multiple -f values.yaml with helm, the right-most file on the command-line will overwrite the values from the previous ones, effectively taking precedence over the values from the other file.

Describe alternatives you've considered

There is also another issue with the current implementation when let's say a user sets a value using one of chectl switches (let's say chectl server:deploy --domain=mydomain.com and also sets it again in values.yaml

global:
  cheDomain: "anotherdomain.net"

which one will be used? The user has no clue that chectl is flatting the yaml and uses --set, and even if he does, he doesn't know the order that will be used for the --set switches. So we end up with something like:

helm upgrade --install che --force --namespace che 
--set global.cheDomain=mydomain.com 
--set xxx=xxx
--set yyy=yyy
...
--set global.cheDomain=anotherdomain.net
...

So it creates confusion in my opinion. A solution would be to leave the code as-is for the chectl arguments (pass them as --set arguments), so they will take priority over what's in values.yaml. The documentation could be reflected accordingly to explain the priority, so the user can make a decision on overwriting or not what's in values.yaml.

Additional context

@cccs-eric cccs-eric added the kind/enhancement A feature request - must adhere to the feature request template. label Nov 25, 2020
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Nov 25, 2020
@tolusha tolusha added severity/P1 Has a major impact to usage or development of the system. area/chectl Issues related to chectl, the CLI of Che and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Nov 25, 2020
@tolusha
Copy link
Contributor

tolusha commented Nov 25, 2020

@cccs-eric
Thank you for investigation, we will take that into account.

@tolusha tolusha added this to the 7.24 milestone Dec 2, 2020
@tolusha tolusha mentioned this issue Dec 7, 2020
56 tasks
@mmorhun mmorhun self-assigned this Dec 9, 2020
@mmorhun
Copy link
Contributor

mmorhun commented Dec 14, 2020

Resolved by che-incubator/chectl#1026

@mmorhun mmorhun closed this as completed Dec 14, 2020
@mmorhun
Copy link
Contributor

mmorhun commented Dec 14, 2020

@cccs-eric thank you for detailed report and investigation !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chectl Issues related to chectl, the CLI of Che kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

4 participants