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"
@@ -39,7 +38,6 @@ type diffCmd struct {
 	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
@@ -89,7 +87,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"
+	clientOnly := d.dryRunMode == "true" || d.dryRunMode == "client"
 	return !clientOnly
 }
 
@@ -111,10 +109,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 +150,12 @@ 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 != "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 +231,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 = "true"
 	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)")