-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Adding --set-string flag to force string values. #3599
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
d6165af
to
6bd587c
Compare
@arturo-c Thanks for the contribution! I will take a look at this on Monday. If for some reason I don't get around to it, feel free to ping me. Sorry for the delay on this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change to make sure we don't break publicly exposed APIs, otherwise this looks pretty straightforward and you added unit tests 🎉
pkg/strvals/parser.go
Outdated
@@ -54,9 +54,9 @@ func Parse(s string) (map[string]interface{}, error) { | |||
// | |||
// If the strval string has a key that exists in dest, it overwrites the | |||
// dest version. | |||
func ParseInto(s string, dest map[string]interface{}) error { | |||
func ParseInto(s string, dest map[string]interface{}, stringBoolean bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be clearer and won't break the current API if you add a new method called ParseIntoString
. The other private methods can still use a bool
, but for someone consuming this package, they won't need to change how they are calling the methods. There might even be a better idea from one of the other maintainers as well. ParseIntoString
seems pretty simple though
cbaad12
to
5300a29
Compare
@thomastaylor312 thanks for the input, I had to add two public methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one more thing I missed in initial review that I caught during some testing. After that, this should be good to go with one more review
cmd/helm/template.go
Outdated
@@ -96,6 +97,7 @@ func newTemplateCmd(out io.Writer) *cobra.Command { | |||
f.VarP(&t.valueFiles, "values", "f", "specify values in a YAML file (can specify multiple)") | |||
f.StringVar(&t.namespace, "namespace", "", "namespace to install the release into") | |||
f.StringArrayVar(&t.values, "set", []string{}, "set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)") | |||
f.StringArrayVar(&t.values, "set-string", []string{}, "set STRING values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a copy/paste error. This should be t.stringValues
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, updated 👍
@arturo-c It looks like this should be rebased against master because it is overwriting changes on helm/master |
769d05e
to
18e27d2
Compare
@thomastaylor312 cool, not sure what happened there, but fixed the commit. |
@arturo-c So because this is labeled as a large, it will need 2 reviews before merge. We have the dev call tomorrow, so I will make sure someone reviews it. Great work on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a good approach. Thanks!
@arturo-c
not yet released i guess? |
you can test it with either the canary releases or v2.9.0-rc4 :) |
Btw when is this supposed to be released? I checked with 2.9.1 and gett this:
|
@babatundebusari and @OndraZizka What is your:
Then I recommend first updating your client version to |
I think both server and client were 2.9.1 in my case. |
Anyway - does the introduction of |
This resolves an issue where numeric values intended to be used as strings could be interpreted as float64 values, causing varius things to fail in unexpected ways. This fix mirrors the changes in the following PR to Helm itself: helm/helm#3599 Closes databus23#83
Adding --set-string flag to force string values.
This addresses the problem of not being able to pass a long int value via the --set flag, and helm converting it to float64 scientific notation, one can then manipulate the string as needed inside the helm templates:
Example usage:
helm install --set long_string_value=1234567890
results in value once in helm template of:1.23456789e+09
with new flag:
helm install --set-string long_string_value=1234567890
-> value ="1234567890"
Addresses these github issues:
#1707
#1694
#3155
#3001
#3246
This is my first dive at go, so please be gentle if I'm doing something completely wrong :)