Tweaking new dry-run internal handling

There are a few changes to the new dry-run handling:

1. Some documentation is added to help clarify what is happening
   and what is expected.
2. DryRun is never changed by internal handling. If an API user
   sets the property it is not changed by our code.
3. The behavior on contacting the server with false/none is made
   consistent between install and upgrade.

Signed-off-by: Matt Farina <matt.farina@suse.com>

cmd/helm/install.go

@@ -142,6 +142,9 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			}
 			client.SetRegistryClient(registryClient)
 
+			// This is for the case where "" is specifically passed in as a
+			// value. When there is no value passed in NoOptDefVal will be used
+			// and it is set to client. See addInstallFlags.
 			if client.DryRunOption == "" {
 				client.DryRunOption = "none"
 			}
@@ -163,6 +166,11 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 
 func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Install, valueOpts *values.Options) {
 	f.BoolVar(&client.CreateNamespace, "create-namespace", false, "create the release namespace if not present")
+	// --dry-run options with expected outcome:
+	// - Not set means no dry run and server is contacted.
+	// - Set with no value, a value of client, or a value of true and the server is not contacted
+	// - Set with a value of false, none, or false and the server is contacted
+	// The true/false part is meant to reflect some legacy behavior while none is equal to "".
 	f.StringVar(&client.DryRunOption, "dry-run", "", "simulate an install. If --dry-run is set with no option being specified or as '--dry-run=client', it will not attempt cluster connections. Setting '--dry-run=server' allows attempting cluster connections.")
 	f.Lookup("dry-run").NoOptDefVal = "client"
 	f.BoolVar(&client.Force, "force", false, "force resource updates through a replacement strategy")

cmd/helm/template.go

@@ -79,6 +79,9 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			}
 			client.SetRegistryClient(registryClient)
 
+			// This is for the case where "" is specifically passed in as a
+			// value. When there is no value passed in NoOptDefVal will be used
+			// and it is set to client. See addInstallFlags.
 			if client.DryRunOption == "" {
 				client.DryRunOption = "true"
 			}

cmd/helm/upgrade.go

@@ -96,6 +96,9 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			}
 			client.SetRegistryClient(registryClient)
 
+			// This is for the case where "" is specifically passed in as a
+			// value. When there is no value passed in NoOptDefVal will be used
+			// and it is set to client. See addInstallFlags.
 			if client.DryRunOption == "" {
 				client.DryRunOption = "none"
 			}

pkg/action/install.go

@@ -233,13 +233,8 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 		return nil, err
 	}
 
-	// Determine dry run behavior
-	if i.DryRun || i.DryRunOption == "client" || i.DryRunOption == "server" || i.DryRunOption == "true" {
-		i.DryRun = true
-	}
-
 	var interactWithRemote bool
-	if !i.DryRun || i.DryRunOption == "server" || i.DryRunOption == "none" || i.DryRunOption == "false" {
+	if !i.isDryRun() || i.DryRunOption == "server" || i.DryRunOption == "none" || i.DryRunOption == "false" {
 		interactWithRemote = true
 	}
 
@@ -247,7 +242,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	// contacts the upstream server and builds the capabilities object.
 	if crds := chrt.CRDObjects(); !i.ClientOnly && !i.SkipCRDs && len(crds) > 0 {
 		// On dry run, bail here
-		if i.DryRun {
+		if i.isDryRun() {
 			i.cfg.Log("WARNING: This chart or one of its subcharts contains CRDs. Rendering may fail or contain inaccuracies.")
 		} else if err := i.installCRDs(crds); err != nil {
 			return nil, err
@@ -281,7 +276,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	}
 
 	// special case for helm template --is-upgrade
-	isUpgrade := i.IsUpgrade && i.DryRun
+	isUpgrade := i.IsUpgrade && i.isDryRun()
 	options := chartutil.ReleaseOptions{
 		Name:      i.ReleaseName,
 		Namespace: i.Namespace,
@@ -338,7 +333,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	}
 
 	// Bail out here if it is a dry run
-	if i.DryRun {
+	if i.isDryRun() {
 		rel.Info.Description = "Dry run complete"
 		return rel, nil
 	}
@@ -398,6 +393,14 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	}
 }
 
+// isDryRun returns true if Upgrade is set to run as a DryRun
+func (i *Install) isDryRun() bool {
+	if i.DryRun || i.DryRunOption == "client" || i.DryRunOption == "server" || i.DryRunOption == "true" {
+		return true
+	}
+	return false
+}
+
 func (i *Install) performInstall(c chan<- resultMessage, rel *release.Release, toBeAdopted kube.ResourceList, resources kube.ResourceList) {
 
 	// pre-install hooks
@@ -512,7 +515,7 @@ func (i *Install) availableName() error {
 		return errors.Wrapf(err, "release name %q", start)
 	}
 	// On dry run, bail here
-	if i.DryRun {
+	if i.isDryRun() {
 		return nil
 	}
 

pkg/action/upgrade.go

@@ -149,11 +149,6 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
 		return nil, errors.Errorf("release name is invalid: %s", name)
 	}
 
-	// Determine dry run behavior
-	if u.DryRun || u.DryRunOption == "client" || u.DryRunOption == "server" || u.DryRunOption == "true" {
-		u.DryRun = true
-	}
-
 	u.cfg.Log("preparing upgrade for %s", name)
 	currentRelease, upgradedRelease, err := u.prepareUpgrade(name, chart, vals)
 	if err != nil {
@@ -169,7 +164,7 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
 	}
 
 	// Do not update for dry runs
-	if !u.DryRun {
+	if !u.isDryRun() {
 		u.cfg.Log("updating status for upgraded release for %s", name)
 		if err := u.cfg.Releases.Update(upgradedRelease); err != nil {
 			return res, err
@@ -179,6 +174,14 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, chart *chart.
 	return res, nil
 }
 
+// isDryRun returns true if Upgrade is set to run as a DryRun
+func (u *Upgrade) isDryRun() bool {
+	if u.DryRun || u.DryRunOption == "client" || u.DryRunOption == "server" || u.DryRunOption == "true" {
+		return true
+	}
+	return false
+}
+
 // prepareUpgrade builds an upgraded release for an upgrade operation.
 func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[string]interface{}) (*release.Release, *release.Release, error) {
 	if chart == nil {
@@ -249,7 +252,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 
 	// Determine whether or not to interact with remote
 	var interactWithRemote bool
-	if !u.DryRun || u.DryRunOption == "server" {
+	if !u.isDryRun() || u.DryRunOption == "server" || u.DryRunOption == "none" || u.DryRunOption == "false" {
 		interactWithRemote = true
 	}
 
@@ -332,7 +335,7 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR
 	})
 
 	// Run if it is a dry run
-	if u.DryRun {
+	if u.isDryRun() {
 		u.cfg.Log("dry run for %s", upgradedRelease.Name)
 		if len(u.Description) > 0 {
 			upgradedRelease.Info.Description = u.Description