-
Notifications
You must be signed in to change notification settings - Fork 226
Add --set-values flags and migrate --set to alias new --set-variables #4466
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
base: main
Are you sure you want to change the base?
Conversation
…e --set description to indicate it's now an alias for this flag Signed-off-by: Kit Patella <[email protected]>
Signed-off-by: Kit Patella <[email protected]>
✅ Deploy Preview for zarf-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Signed-off-by: Kit Patella <[email protected]>
Signed-off-by: Kit Patella <[email protected]>
Signed-off-by: Kit Patella <[email protected]>
Signed-off-by: Kit Patella <[email protected]>
brandtkeller
left a comment
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.
migration makes sense - added some thoughts around guardrails natively available in cobra that we might consider.
|
|
||
| // Init package set variable flags | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), lang.CmdInitFlagSet) | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), "Alias for --set-variables") |
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.
Should we explicitly mark these as deprecated?
cmd.Flags().MarkDeprecated("set", "use --set-variables instead")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.
Rather than having a separate command as an alias that we mark as deprecated or hide, I think it would be better to use the builtin cobra functionality for aliasing a command. This leads to a more intuitive experience around viper config, and will lead to an easier time aliasing --set-values to --set in the future in the event that variables is fully replaced
For instance
cmd.Flags().StringToStringVarP(&o.setVariables, "set-variables", "set", v.GetStringMapString(VPkgCreateSet), lang.CmdPackageCreateFlagSetVariables)
| // Init package set variable flags | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), lang.CmdInitFlagSet) | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), "Alias for --set-variables") | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set-variables", v.GetStringMapString(VPkgDeploySet), lang.CmdInitFlagSetVariables) |
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.
Given this alias - do we need to mark these flags as mutually exclusive? I understand this to create a last-write-wins scenario if there were to be both used?
|
|
||
| // Init package set variable flags | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), lang.CmdInitFlagSet) | ||
| cmd.Flags().StringToStringVar(&o.setVariables, "set", v.GetStringMapString(VPkgDeploySet), "Alias for --set-variables") |
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.
Rather than having a separate command as an alias that we mark as deprecated or hide, I think it would be better to use the builtin cobra functionality for aliasing a command. This leads to a more intuitive experience around viper config, and will lead to an easier time aliasing --set-values to --set in the future in the event that variables is fully replaced
For instance
cmd.Flags().StringToStringVarP(&o.setVariables, "set-variables", "set", v.GetStringMapString(VPkgCreateSet), lang.CmdPackageCreateFlagSetVariables)
| cmd.Flags().StringToStringVar(&o.createSetVariables, "create-set", v.GetStringMapString(VPkgCreateSet), lang.CmdDevFlagSet) | ||
| cmd.Flags().StringToStringVar(&o.deploySetVariables, "deploy-set", v.GetStringMapString(VPkgDeploySet), lang.CmdPackageDeployFlagSet) | ||
| cmd.Flags().StringToStringVar(&o.createSetVariables, "create-set", v.GetStringMapString(VPkgCreateSet), "Alias for --create-set-variables") | ||
| cmd.Flags().StringToStringVar(&o.createSetVariables, "create-set-variables", v.GetStringMapString(VPkgCreateSet), lang.CmdDevFlagSetVariables) |
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.
IMO we shouldn't add the variables verbiage to create time templates. Generally I think we should refer to these as package templates. I just noticed that Zarf is inconsistent right now since we call them package variables in this flag, and package templates in our docs. Still, I don't think we'll ever have two types of package templates, at least not in the same API version, so I'm not sure extra verbosity is helpful.
| VPkgDeployNamespace = "package.deploy.namespace" | ||
| VPkgRetries = "package.deploy.retries" | ||
| VPkgDeployValues = "package.deploy.values" | ||
| VPkgDeploySetValues = "package.deploy.set_values" |
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.
Currently both package.deploy.values and package.deploy.set_values are in use. Not sure which I prefer, but we should only use one.
| v := getViper() | ||
| o.setVariables = helpers.TransformAndMergeMap( | ||
| v.GetStringMapString(VPkgDeploySet), o.setVariables, strings.ToUpper) | ||
| o.setValues = helpers.TransformAndMergeMap( |
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.
When there is no transform happening, this function is simply a maps.Copy, lets just use that instead.
Description
This PR adds the
--set-valuesflag so users can directly override values via the CLI, and begins the path to migrating--set. To do so, we add the--set-variablesflag (or versions of it like--deploy-set-variables) and change the description of--setto indicate that it now aliases --set-variables. This alias is just a documentation change in the CLI help text: the flags function identically.Related Issue
Fixes #4224
Checklist before merging