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

Fix the values processing on cli #282

Merged
merged 1 commit into from
Jul 23, 2020
Merged

Fix the values processing on cli #282

merged 1 commit into from
Jul 23, 2020

Conversation

yogeshmundada
Copy link
Contributor

@yogeshmundada yogeshmundada commented Jul 10, 2020

Problem: rancher/rancher#27841

Solution: Instead of parsing the yaml as a map[string]string that would lose type information about bool variables, now parsing it as map[string]interface{}. When flattening the structure, a separate case for bool type is created so that other map members are converted to string except bool.

@yogeshmundada yogeshmundada requested review from luthermonson and removed request for dramich July 12, 2020 21:22
@yogeshmundada
Copy link
Contributor Author

With this fix, most of the issues will be fixed.
There is one discrepancy remaining. If the UI is edited for something like "abc: yes", then it will treat it as a string.
However, when installed from the CLI, it would be converted to true. A related issue can be fond here:
uber-go/config#74

However, without addition of special tags around these specific values, there does not seem to be a straightforward way to fix this.

Copy link

@brendarearden brendarearden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after talking with Yogesh, it seems like the yaml parser is keeping us from recreating the ui behavior exactly so strings of bools will convert to bools which is not the case in the ui which is a minor difference. LGTM!

Copy link
Contributor

@dramich dramich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this fix will introduce a new bug to the parsing which changes values. This needs a different approach to fully fix the issue.

@yogeshmundada
Copy link
Contributor Author

yogeshmundada commented Jul 15, 2020

Using this fix will introduce a new bug to the parsing which changes values. This needs a different approach to fully fix the issue.

the "newly introduced bug" is also present in v2.3.2 (except in the answers sections since the past code put everything from values into answers). The reason behind that is we make a library call yaml.Unmarshal() that already converts an unquoted

yes

to a boolean value. A quoted one would be retained as a string.

From my understanding of your concern, we don't want incorrect values to be parsed by helm when it upgrades an app.
helm code to do so is at:
https://github.com/rancher/helm/blob/master/pkg/chartutil/values.go#L119

If we unmarshal the values, we see that it would still result in the same conversion (yes -> true) as below:
https://play.golang.org/p/TZrDkoZqpUm

Please let me know if there is something I am missing. I agree that if someone downloads valuesYAML that is edited in UI and diffs it against the one uploaded from the CLI then there would be a mismatch. However, apart from that, helm should not have any problem loading those values as the playground and gh links above show.

@yogeshmundada yogeshmundada merged commit 7b2f5fd into rancher:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants