diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 082d2a30..09b50ddd 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -7,13 +7,13 @@ import ( "fmt" "log" "os" + "slices" "strconv" "strings" jsonpatch "github.com/evanphx/json-patch" jsoniterator "github.com/json-iterator/go" "github.com/spf13/cobra" - "github.com/spf13/pflag" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/cli" "helm.sh/helm/v3/pkg/kube" @@ -29,6 +29,14 @@ import ( "github.com/databus23/helm-diff/v3/manifest" ) +var ( + validDryRunValues = []string{"server", "client", "true", "false"} +) + +const ( + dryRunNoOptDefVal = "client" +) + type diffCmd struct { release string chart string @@ -38,8 +46,6 @@ type diffCmd struct { devel bool disableValidation bool disableOpenAPIValidation bool - dryRunMode string - dryRunModeSpecified bool enableDNS bool namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace. valueFiles valueFiles @@ -62,6 +68,14 @@ type diffCmd struct { kubeVersion string useUpgradeDryRun bool diff.Options + + // dryRunMode can take the following values: + // - "none": no dry run is performed + // - "client": dry run is performed without remote cluster access + // - "server": dry run is performed with remote cluster access + // - "true": same as "client" + // - "false": same as "none" + dryRunMode string } func (d *diffCmd) isAllowUnreleased() bool { @@ -73,10 +87,9 @@ func (d *diffCmd) isAllowUnreleased() bool { // clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree. // -// helm-diff basically have 3 modes of operation: -// 1. no cluster access at all when --dry-run, --dry-run=client, or --dry-run=true is specified. -// 2. basic cluster access when --dry-run is not specified. -// 3. extra cluster access when --dry-run=server is specified. +// helm-diff basically have 2 modes of operation: +// 1. without cluster access at all when --dry-run=true or --dry-run=client is specified. +// 2. with cluster access when --dry-run is unspecified, false, or server. // // clusterAccessAllowed returns true when the mode is either 2 or 3. // @@ -89,8 +102,7 @@ func (d *diffCmd) isAllowUnreleased() bool { // // See also https://github.com/helm/helm/pull/9426#discussion_r1181397259 func (d *diffCmd) clusterAccessAllowed() bool { - clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client" - return !clientOnly + return d.dryRunMode == "none" || d.dryRunMode == "false" || d.dryRunMode == "server" } const globalUsage = `Show a diff explaining what a helm upgrade would change. @@ -111,10 +123,9 @@ func newChartCommand() *cobra.Command { unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == "true" cmd := &cobra.Command{ - Use: "upgrade [flags] [RELEASE] [CHART]", - Short: "Show a diff explaining what a helm upgrade would change.", - Long: globalUsage, - DisableFlagParsing: true, + Use: "upgrade [flags] [RELEASE] [CHART]", + Short: "Show a diff explaining what a helm upgrade would change.", + Long: globalUsage, Example: strings.Join([]string{ " helm diff upgrade my-release stable/postgresql --values values.yaml", "", @@ -153,71 +164,14 @@ func newChartCommand() *cobra.Command { "# Read the flag usage below for more information on --context.", "HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog", }, "\n"), + Args: func(cmd *cobra.Command, args []string) error { + return checkArgsLength(len(args), "release name", "chart path") + }, RunE: func(cmd *cobra.Command, args []string) error { - // Note that we can't just move this block to PersistentPreRunE, - // because cmd.SetArgs(args) does not persist between PersistentPreRunE and RunE. - // The choice is between: - // 1. Doing this in RunE - // 2. Doing this in PersistentPreRunE, saving args somewhere, and calling cmd.SetArgs(args) again in RunE - // 2 is more complicated without much benefit, so we choose 1. - { - const ( - dryRunUsage = "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation." + - " --dry-run=server enables the cluster access with helm-get and the lookup template function." - ) - - cmdFlags := cmd.Flags() - - // see: https://github.com/databus23/helm-diff/issues/537 - cmdFlags.ParseErrorsWhitelist.UnknownFlags = unknownFlags - - legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError) - legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage) - if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun { - diff.dryRunModeSpecified = true - args = legacyDryRunFlagSet.Args() - } else { - cmdFlags.StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage) - } - - // Here we parse the flags ourselves so that we can support - // both --dry-run and --dry-run=ARG syntaxes. - // - // If you don't do this, then cobra will treat --dry-run as - // a "flag needs an argument: --dry-run" error. - // - // This works becase we have `DisableFlagParsing: true`` above. - // Never turn that off, or you'll get the error again. - if err := cmdFlags.Parse(args); err != nil { - return err - } - - args = cmdFlags.Args() - - if !diff.dryRunModeSpecified { - dryRunModeSpecified := cmd.Flags().Changed("dry-run") - diff.dryRunModeSpecified = dryRunModeSpecified - - if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") { - return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode) - } - } - - cmd.SetArgs(args) - - // We have to do this here because we have to sort out the - // --dry-run flag ambiguity to determine the args to be checked. - // - // In other words, we can't just do: - // - // cmd.Args = func(cmd *cobra.Command, args []string) error { - // return checkArgsLength(len(args), "release name", "chart path") - // } - // - // Because it seems to take precedence over the PersistentPreRunE - if err := checkArgsLength(len(args), "release name", "chart path"); err != nil { - return err - } + if diff.dryRunMode == "" { + diff.dryRunMode = "none" + } else if !slices.Contains(validDryRunValues, diff.dryRunMode) { + return fmt.Errorf("flag %q must take a bool value or either %q or %q, but got %q", "dry-run", "client", "server", diff.dryRunMode) } // Suppress the command usage on error. See #77 for more info @@ -293,6 +247,9 @@ func newChartCommand() *cobra.Command { f.BoolVar(&diff.devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored.") f.BoolVar(&diff.disableValidation, "disable-validation", false, "disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install") f.BoolVar(&diff.disableOpenAPIValidation, "disable-openapi-validation", false, "disables rendered templates validation against the Kubernetes OpenAPI Schema") + f.StringVar(&diff.dryRunMode, "dry-run", "", "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation."+ + " --dry-run=server enables the cluster access with helm-get and the lookup template function.") + f.Lookup("dry-run").NoOptDefVal = dryRunNoOptDefVal f.BoolVar(&diff.enableDNS, "enable-dns", false, "enable DNS lookups when rendering templates") f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path") f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)") diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index e779599b..6f6752c5 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -9,39 +9,44 @@ func TestIsRemoteAccessAllowed(t *testing.T) { expected bool }{ { - name: "no flags", - cmd: diffCmd{}, + name: "no flags", + cmd: diffCmd{ + dryRunMode: "none", + }, expected: true, }, { - name: "legacy explicit dry-run flag", + name: "legacy explicit dry-run=true flag", cmd: diffCmd{ - dryRunModeSpecified: true, - dryRunMode: "true", + dryRunMode: "true", }, expected: false, }, + { + name: "legacy explicit dry-run=false flag", + cmd: diffCmd{ + dryRunMode: "false", + }, + expected: true, + }, { name: "legacy empty dry-run flag", cmd: diffCmd{ - dryRunModeSpecified: true, - dryRunMode: "", + dryRunMode: dryRunNoOptDefVal, }, expected: false, }, { name: "server-side dry-run flag", cmd: diffCmd{ - dryRunModeSpecified: true, - dryRunMode: "server", + dryRunMode: "server", }, expected: true, }, { name: "client-side dry-run flag", cmd: diffCmd{ - dryRunModeSpecified: true, - dryRunMode: "client", + dryRunMode: "client", }, expected: false, }, diff --git a/go.mod b/go.mod index a9e201d3..e2974c65 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/evanphx/json-patch v5.8.1+incompatible github.com/json-iterator/go v1.1.12 github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d - github.com/pkg/errors v0.9.1 + github.com/pkg/errors v0.9.1 // indirect github.com/spf13/cobra v1.8.0 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.8.4