Remake dry-run=server optional

This is a follow-up on #499 which redoes it without relying on DisableParsing. DisableParsing turned out to break far many things than I imagined!

Author: mumoshu

cmd/upgrade.go

@@ -13,7 +13,6 @@ import (
 	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 +28,10 @@ import (
 	"github.com/databus23/helm-diff/v3/manifest"
 )
 
+const (
+	dryRunNoOptDefVal = "client"
+)
+
 type diffCmd struct {
 	release                  string
 	chart                    string
@@ -38,8 +41,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 +63,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 +82,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 +97,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 +118,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 +159,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 diff.dryRunMode != "true" && diff.dryRunMode != "false" && diff.dryRunMode != "client" && diff.dryRunMode != "server" {
+				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 +242,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)")

cmd/upgrade_test.go

@@ -9,39 +9,51 @@ 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: "client-side dry-run flag",
+			cmd: diffCmd{
+				dryRunMode: "client",
 			},
 			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,
 		},