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
Add diffArgs to helmDefaults #1019
Conversation
Signed-off-by: Yuuki Takahashi <20282867+yktakaha4@users.noreply.github.com>
c7c23f2
to
119f1f7
Compare
pkg/argparser/args.go
Outdated
@@ -83,7 +83,7 @@ func analyzeArgs(am *argMap, args string) { | |||
} | |||
} | |||
|
|||
func GetArgs(args string, state *state.HelmState) []string { | |||
func GetArgs(args string, state *state.HelmState, withDiffArgs bool) []string { |
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.
@ yktakaha4 We can make this func more flexible. there will be more args like applyArgs, sync Args. WDYT?
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.
@yxxhero Thank you for your review.
Should I modify it so that I can get the type of subcommand currently being executed from HelmState
, or define a struct like GetArgsOptions
?
I would be happy to hear your opinions on how to make it more maintainable.
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.
@yktakaha4 we can try to define a struct like GetArgsOptions.
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.
Fixed to use GetArgsOptions.
In the current usage of GetArgs, It seems possible to change the interface to GetArgs(c ApplyConfigProvider, state *state.HelmState, opts *GetArgsOptions)
and decide whether to include diffArgs inside GetArgs.
Should I make this change?
Lines 1372 to 1375 in a08b986
// join --args and --diff-args together to one string. | |
args := strings.Join([]string{c.Args(), c.DiffArgs()}, " ") | |
argsOpts := &argparser.GetArgsOptions{WithDiffArgs: true} | |
helm.SetExtraArgs(argparser.GetArgs(args, r.state, argsOpts)...) |
Edit: I looked at the code again and realized that making the above changes requires more abstraction to the ConfigProvider, so I'm retracting it.
Signed-off-by: Yuuki Takahashi <20282867+yktakaha4@users.noreply.github.com>
* Add diffArgs to helmDefaults Signed-off-by: Yuuki Takahashi <20282867+yktakaha4@users.noreply.github.com>
This OSS always helps me. Thank you to all the maintainers.
In my use case, I want to keep
diffArgs
inhelmfile.yaml
. (Added in #959)I think this is useful if you want to specify additional arguments to
helm-diff
only when using a specifichelmfile.yaml
.(It is now possible to version control argument specifications for specific charts)
Please let me know if there is anything else I need to do to fix it.
For example,
Add
helmDefaults.diffArgs
tohelmfile.yaml
Options are set for
helm-diff
onlyhelmfile diff
andhelmfile apply
.Other subcommands, such as
helmfile template
, are not affected.