From 96e33e2773bb00a82dc94fcdb73950e08dd4229d Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Thu, 20 Jul 2023 14:26:46 -0400 Subject: [PATCH] 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 --- cmd/helm/install.go | 8 ++++++++ cmd/helm/template.go | 3 +++ cmd/helm/upgrade.go | 3 +++ pkg/action/install.go | 23 +++++++++++++---------- pkg/action/upgrade.go | 19 +++++++++++-------- 5 files changed, 38 insertions(+), 18 deletions(-) diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 935f1f990..bc095de77 100644 --- a/cmd/helm/install.go +++ b/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") diff --git a/cmd/helm/template.go b/cmd/helm/template.go index 2cf89131b..f8cd8268b 100644 --- a/cmd/helm/template.go +++ b/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" } diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 5e23f8347..7e6082782 100644 --- a/cmd/helm/upgrade.go +++ b/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" } diff --git a/pkg/action/install.go b/pkg/action/install.go index e24dde070..1860b32f3 100644 --- a/pkg/action/install.go +++ b/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 } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 4918f1635..8ee6ed881 100644 --- a/pkg/action/upgrade.go +++ b/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