Navigation Menu

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

feat: add --set-json flag to set json values. #10693

Merged
merged 1 commit into from Aug 31, 2022

Conversation

lucadirocco
Copy link
Contributor

@lucadirocco lucadirocco commented Feb 22, 2022

When used with helm install, helm template, helm upgrade, it enables
to set json values (scalars/objects/arrays) from the command line.

Closes #10428

Signed-off-by: Luca Di Rocco lucadirocco@gmail.com

What this PR does / why we need it:
This PR allows to set any key of the exposed values (scalars, objects, arrays) of a chart, by using a json value on the command line.
It become easier and more concise to update entire portions of the exposed values, as json data structures on command line, rather than setting the value of each individual leaf attribute of the data structure (#10428 provides an example)

Special notes for your reviewer:
The implementation has tried to reuse as much as possible the existing code and the standard libraries, and is quite straightforward.
There are some obvious changes in cmd/helm/flags.go and pkg/cli/values/options.go in order to include the new option flag (--set-json)
The parser logic from pkg/strvals/parser.go is reused, with an extra field in the parser struct (isjsonval bool, true only for --set-json) and some extra code in the following two functions, to parse a json value when a "=" char is met:
func (t *parser) key(data map[string]interface{}) (reterr error)
func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error)

If applicable:

  • this PR contains documentation
    Little updates in the description constants of cmd/helm/install.go and cmd/helm/upgrade.go to.
    Together with the info from cmd/helm/flags.go, this should update the documentation when "helm docs --type markdown --generate-headers" is executed.
  • this PR contains unit tests
    Some extra unit tests with function TestParseJSON in pkg/strvals/parser_test.go
  • this PR has been tested for backwards compatibility
    All unit test run, I don't know if something else is required. Being this a new feature, there are no changes in existing functionalities.
    Please note that I had also to remove the ending new line '\n' from an errors.Errorf statement in the
    func compare(actual []byte, filename string) error, in file ./internal/test/test.go, because go lint was failing and also tests were not running.

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 22, 2022
expect map[string]interface{}
err bool
}{
{ // set json scalars values, and replace one existing key
Copy link
Contributor

@joejulian joejulian Mar 1, 2022

Choose a reason for hiding this comment

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

What happens if the flag is specified twice with conflicting values or subsets of values?

--set-json='foo={"key1":"value1","key2":"value2"}' --set-json='foo.key2="bar"'

or

--set-json='foo=["one", "two", "three"]' --set-json='foo=["four"]'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly to the other --set-x flags, for each key the rightmost assignment prevails.
This implies that with --set-json='foo={"key1":"value1","key2":"value2"}' --set-json='foo.key2="bar"' foo is set to {"key1":"value1","key2":"bar"}
while with --set-json='foo=["one", "two", "three"]' --set-json='foo=["four"]' foo is set to ["four"]

Copy link
Contributor

@joejulian joejulian Mar 2, 2022

Choose a reason for hiding this comment

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

Should this be explicitly called out in documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can add some more info/examples to install.go and upgrade.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joejulian
I have checked that in cmd/helm/install.go (const installDesc) it is already explained the behavior of the --set flag when it is specified multiple times.
I have added the above examples in order to highlight a similar behavior with --set-json:

You can specify the ‘–set’ flag multiple times. The priority will be given to the last (right-most) set specified. For example, if both ‘bar’ and ‘newbar’ values are set for a key called ‘foo’, the ‘newbar’ value would take precedence:

$ helm install --set foo=bar --set foo=newbar  myredis ./redis

Similarly, in the following example ‘foo’ is set to ‘[“four”]’:

$ helm install --set-json='foo=["one", "two", "three"]' --set-json='foo=["four"]' myredis ./redis

And in the following example, ‘foo’ is set to ‘{“key1”:“value1”,“key2”:“bar”}’:

$ helm install --set-json='foo={"key1":"value1","key2":"value2"}' --set-json='foo.key2="bar"' myredis ./redis

When used with helm install, helm template, helm upgrade, it enables
to set json values (scalars/objects/arrays) from the command line.

Closes helm#10428

Signed-off-by: Luca Di Rocco <lucadirocco@gmail.com>
Copy link
Contributor

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not an approver.

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @lucadirocco

@lucadirocco
Copy link
Contributor Author

LGTM, thanks @lucadirocco

Thank you @hickeyma

@mattfarina mattfarina modified the milestones: 3.9.0, 3.10.0 May 18, 2022
@hickeyma
Copy link
Contributor

This need approval from another maintainer to merge.

Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing json objects/arrays as values through --set
5 participants