Pass --revision to helm diff upgrade (#1046)

* fix for #1045

* fix lint

Author: sepich

README.md

@@ -168,6 +168,7 @@ Flags:
       --reset-then-reuse-values                  reset the values to the ones built into the chart, apply the last release's values and merge in any new values. If '--reset-values' or '--reuse-values' is specified, this is ignored
       --reset-values                             reset the values to the ones built into the chart and merge in any new values
       --reuse-values                             reuse the last release's values and merge in any new values. If '--reset-values' is specified, this is ignored
+      --revision int                             revision of the release to use as the diff baseline instead of the newest one
       --server-side string                       must be "true", "false" or "auto". Object updates run in the server instead of the client ("auto" defaults the value from the previous chart release's method) (default "auto")
       --set stringArray                          set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
       --set-file stringArray                     set values from respective files specified via the command line (can specify multiple or separate values with commas: key1=path1,key2=path2)
@@ -349,6 +350,7 @@ Flags:
       --reset-then-reuse-values                  reset the values to the ones built into the chart, apply the last release's values and merge in any new values. If '--reset-values' or '--reuse-values' is specified, this is ignored
       --reset-values                             reset the values to the ones built into the chart and merge in any new values
       --reuse-values                             reuse the last release's values and merge in any new values. If '--reset-values' is specified, this is ignored
+      --revision int                             revision of the release to use as the diff baseline instead of the newest one
       --server-side string                       must be "true", "false" or "auto". Object updates run in the server instead of the client ("auto" defaults the value from the previous chart release's method) (default "auto")
       --set stringArray                          set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)
       --set-file stringArray                     set values from respective files specified via the command line (can specify multiple or separate values with commas: key1=path1,key2=path2)

cmd/helm.go

@@ -131,44 +131,43 @@ func compatibleHelm3Version() error {
 	return nil
 }
 
-func getRelease(release, namespace, kubeContext string) ([]byte, error) {
-	args := []string{"get", "manifest", release}
+// helmGetSubcmd is the `helm get` subcommand used to read data of a deployed release.
+const helmGetSubcmd = "get"
+
+// helmGetArgs builds the arguments for a `helm get <what> <release>` invocation.
+//
+// A revision of 0 means no --revision flag is passed, so helm defaults to the
+// newest revision of the release regardless of its status.
+func helmGetArgs(what, release string, revision int, namespace, kubeContext string) []string {
+	args := []string{helmGetSubcmd, what, release}
+	if revision > 0 {
+		args = append(args, "--revision", strconv.Itoa(revision))
+	}
 	if namespace != "" {
 		args = append(args, "--namespace", namespace)
 	}
 	if kubeContext != "" {
 		args = append(args, "--kube-context", kubeContext)
 	}
-	cmd := exec.Command(os.Getenv("HELM_BIN"), args...)
-	return outputWithRichError(cmd)
+	return args
 }
 
-func getHooks(release, namespace, kubeContext string) ([]byte, error) {
-	args := []string{"get", "hooks", release}
-	if namespace != "" {
-		args = append(args, "--namespace", namespace)
-	}
-	if kubeContext != "" {
-		args = append(args, "--kube-context", kubeContext)
-	}
-	cmd := exec.Command(os.Getenv("HELM_BIN"), args...)
+// getRelease returns the manifest of the given release revision.
+// A revision of 0 means the newest revision.
+func getRelease(release string, revision int, namespace, kubeContext string) ([]byte, error) {
+	cmd := exec.Command(os.Getenv("HELM_BIN"), helmGetArgs("manifest", release, revision, namespace, kubeContext)...)
 	return outputWithRichError(cmd)
 }
 
-func getRevision(release string, revision int, namespace, kubeContext string) ([]byte, error) {
-	args := []string{"get", "manifest", release, "--revision", strconv.Itoa(revision)}
-	if namespace != "" {
-		args = append(args, "--namespace", namespace)
-	}
-	if kubeContext != "" {
-		args = append(args, "--kube-context", kubeContext)
-	}
-	cmd := exec.Command(os.Getenv("HELM_BIN"), args...)
+// getHooks returns the hooks of the given release revision.
+// A revision of 0 means the newest revision.
+func getHooks(release string, revision int, namespace, kubeContext string) ([]byte, error) {
+	cmd := exec.Command(os.Getenv("HELM_BIN"), helmGetArgs("hooks", release, revision, namespace, kubeContext)...)
 	return outputWithRichError(cmd)
 }
 
 func getChart(release, namespace, kubeContext string) (string, error) {
-	args := []string{"get", "all", release, "--template", "{{.Release.Chart.Name}}"}
+	args := []string{helmGetSubcmd, "all", release, "--template", "{{.Release.Chart.Name}}"}
 	if namespace != "" {
 		args = append(args, "--namespace", namespace)
 	}
@@ -425,7 +424,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) {
 }
 
 func (d *diffCmd) writeExistingValues(f *os.File, all bool) error {
-	args := []string{"get", "values", d.release, "--output", "yaml"}
+	args := []string{helmGetSubcmd, "values", d.release, "--output", "yaml"}
 	if all {
 		args = append(args, "--all")
 	}

cmd/helm_test.go

@@ -522,3 +522,63 @@ To connect to your database directly from outside the K8s cluster:
 		})
 	}
 }
+
+func TestHelmGetArgs(t *testing.T) {
+	cases := []struct {
+		name        string
+		what        string
+		release     string
+		revision    int
+		namespace   string
+		kubeContext string
+		expected    []string
+	}{
+		{
+			name:     "manifest without revision omits the flag",
+			what:     "manifest",
+			release:  "myapp",
+			revision: 0,
+			expected: []string{helmGetSubcmd, "manifest", "myapp"},
+		},
+		{
+			name:     "manifest with revision",
+			what:     "manifest",
+			release:  "myapp",
+			revision: 49,
+			expected: []string{helmGetSubcmd, "manifest", "myapp", "--revision", "49"},
+		},
+		{
+			name:     "hooks with revision",
+			what:     "hooks",
+			release:  "myapp",
+			revision: 49,
+			expected: []string{helmGetSubcmd, "hooks", "myapp", "--revision", "49"},
+		},
+		{
+			name:        "revision with namespace and kube context",
+			what:        "manifest",
+			release:     "myapp",
+			revision:    2,
+			namespace:   "myns",
+			kubeContext: "myctx",
+			expected:    []string{helmGetSubcmd, "manifest", "myapp", "--revision", "2", "--namespace", "myns", "--kube-context", "myctx"},
+		},
+		{
+			name:      "negative revision is treated as unset",
+			what:      "manifest",
+			release:   "myapp",
+			revision:  -1,
+			namespace: "myns",
+			expected:  []string{helmGetSubcmd, "manifest", "myapp", "--namespace", "myns"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := helmGetArgs(tc.what, tc.release, tc.revision, tc.namespace, tc.kubeContext)
+			if d := cmp.Diff(tc.expected, actual); d != "" {
+				t.Errorf("unexpected diff: %s", d)
+			}
+		})
+	}
+}

cmd/release.go

@@ -84,7 +84,7 @@ func (d *release) differentiateHelm3() error {
 		namespace1 = strings.Split(release1, "/")[0]
 		release1 = strings.Split(release1, "/")[1]
 	}
-	releaseResponse1, err := getRelease(release1, namespace1, d.kubeContext)
+	releaseResponse1, err := getRelease(release1, 0, namespace1, d.kubeContext)
 	if err != nil {
 		return err
 	}
@@ -99,7 +99,7 @@ func (d *release) differentiateHelm3() error {
 		namespace2 = strings.Split(release2, "/")[0]
 		release2 = strings.Split(release2, "/")[1]
 	}
-	releaseResponse2, err := getRelease(release2, namespace2, d.kubeContext)
+	releaseResponse2, err := getRelease(release2, 0, namespace2, d.kubeContext)
 	if err != nil {
 		return err
 	}

cmd/revision.go

@@ -87,14 +87,14 @@ func (d *revision) differentiateHelm3() error {
 	}
 	switch len(d.revisions) {
 	case 1:
-		releaseResponse, err := getRelease(d.release, namespace, d.kubeContext)
+		releaseResponse, err := getRelease(d.release, 0, namespace, d.kubeContext)
 
 		if err != nil {
 			return err
 		}
 
 		revision, _ := strconv.Atoi(d.revisions[0])
-		revisionResponse, err := getRevision(d.release, revision, namespace, d.kubeContext)
+		revisionResponse, err := getRelease(d.release, revision, namespace, d.kubeContext)
 		if err != nil {
 			return err
 		}
@@ -117,12 +117,12 @@ func (d *revision) differentiateHelm3() error {
 			revision1, revision2 = revision2, revision1
 		}
 
-		revisionResponse1, err := getRevision(d.release, revision1, namespace, d.kubeContext)
+		revisionResponse1, err := getRelease(d.release, revision1, namespace, d.kubeContext)
 		if err != nil {
 			return err
 		}
 
-		revisionResponse2, err := getRevision(d.release, revision2, namespace, d.kubeContext)
+		revisionResponse2, err := getRelease(d.release, revision2, namespace, d.kubeContext)
 		if err != nil {
 			return err
 		}

cmd/rollback.go

@@ -76,15 +76,15 @@ func (d *rollback) backcastHelm3() error {
 		excludes = []string{}
 	}
 	// get manifest of the latest release
-	releaseResponse, err := getRelease(d.release, namespace, d.kubeContext)
+	releaseResponse, err := getRelease(d.release, 0, namespace, d.kubeContext)
 
 	if err != nil {
 		return err
 	}
 
 	// get manifest of the release to rollback
 	revision, _ := strconv.Atoi(d.revisions[0])
-	revisionResponse, err := getRevision(d.release, revision, namespace, d.kubeContext)
+	revisionResponse, err := getRelease(d.release, revision, namespace, d.kubeContext)
 	if err != nil {
 		return err
 	}

cmd/upgrade.go

@@ -72,6 +72,7 @@ type diffCmd struct {
 	extraAPIs                []string
 	kubeVersion              string
 	useUpgradeDryRun         bool
+	revision                 int // 0 = newest, which is what helm returns by default.
 	diff.Options
 
 	// dryRunMode can take the following values:
@@ -111,6 +112,21 @@ func (d *diffCmd) clusterAccessAllowed() bool {
 	return d.dryRunMode == dryRunNone || d.dryRunMode == envFalse || d.dryRunMode == dryRunServer
 }
 
+// validateRevision checks the --revision flag, which is only meaningful when the
+// flag was set and helm-diff is allowed to read the release from the cluster.
+func (d *diffCmd) validateRevision(changed bool) error {
+	if !changed {
+		return nil
+	}
+	if d.revision < 1 {
+		return fmt.Errorf("flag %q must be a positive revision number, but got %d", "revision", d.revision)
+	}
+	if !d.clusterAccessAllowed() {
+		return fmt.Errorf("flag %q requires cluster access, so it cannot be used with --dry-run=%s", "revision", d.dryRunMode)
+	}
+	return nil
+}
+
 const globalUsage = `Show a diff explaining what a helm upgrade would change.
 
 This fetches the currently deployed version of a release
@@ -173,6 +189,10 @@ func newChartCommand() *cobra.Command {
 				return fmt.Errorf("flag %q must be %q, %q or %q, but got %q", "server-side", envTrue, envFalse, serverSideAuto, diff.serverSide)
 			}
 
+			if err := diff.validateRevision(cmd.Flags().Changed("revision")); err != nil {
+				return err
+			}
+
 			// Suppress the command usage on error. See #77 for more info
 			cmd.SilenceUsage = true
 
@@ -259,6 +279,7 @@ func newChartCommand() *cobra.Command {
 	f.BoolVar(&diff.insecureSkipTLSVerify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download")
 	f.BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output")
 	f.BoolVar(&diff.takeOwnership, "take-ownership", false, "if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources")
+	f.IntVar(&diff.revision, "revision", 0, "revision of the release to use as the diff baseline instead of the newest one")
 	f.StringVar(&diff.serverSide, "server-side", serverSideAuto, `must be "true", "false" or "auto". Object updates run in the server instead of the client ("auto" defaults the value from the previous chart release's method)`)
 
 	AddDiffOptions(f, &diff.Options)
@@ -282,11 +303,14 @@ func (d *diffCmd) runHelm3() error {
 	}
 
 	if d.clusterAccessAllowed() {
-		releaseManifest, err = getRelease(d.release, d.namespace, d.kubeContext)
+		releaseManifest, err = getRelease(d.release, d.revision, d.namespace, d.kubeContext)
 	}
 
 	var newInstall bool
 	if err != nil && strings.Contains(err.Error(), "release: not found") {
+		if d.revision > 0 {
+			return fmt.Errorf("Failed to get revision %d of release %s in namespace %s: %w", d.revision, d.release, d.namespace, err)
+		}
 		if d.isAllowUnreleased() {
 			newInstall = true
 			err = nil
@@ -326,7 +350,7 @@ func (d *diffCmd) runHelm3() error {
 	currentSpecs := make(map[string]*manifest.MappingResult)
 	if !newInstall && d.clusterAccessAllowed() {
 		if !d.noHooks && !d.threeWayMerge {
-			hooks, err := getHooks(d.release, d.namespace, d.kubeContext)
+			hooks, err := getHooks(d.release, d.revision, d.namespace, d.kubeContext)
 			if err != nil {
 				return err
 			}

cmd/upgrade_test.go

@@ -205,3 +205,31 @@ func TestServerSideFlagValidation(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateRevision(t *testing.T) {
+	cases := []struct {
+		name       string
+		revision   int
+		changed    bool
+		dryRunMode string
+		expectErr  bool
+	}{
+		{name: "unset", revision: 0, changed: false, dryRunMode: dryRunNone, expectErr: false},
+		{name: "positive revision", revision: 2, changed: true, dryRunMode: dryRunNone, expectErr: false},
+		{name: "positive revision with dry-run=server", revision: 2, changed: true, dryRunMode: dryRunServer, expectErr: false},
+		{name: "explicit zero", revision: 0, changed: true, dryRunMode: dryRunNone, expectErr: true},
+		{name: "negative revision", revision: -1, changed: true, dryRunMode: dryRunNone, expectErr: true},
+		{name: "dry-run=client denies cluster access", revision: 2, changed: true, dryRunMode: dryRunNoOptDefVal, expectErr: true},
+		{name: "dry-run=true denies cluster access", revision: 2, changed: true, dryRunMode: envTrue, expectErr: true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			d := diffCmd{revision: tc.revision, dryRunMode: tc.dryRunMode}
+			err := d.validateRevision(tc.changed)
+			if (err != nil) != tc.expectErr {
+				t.Errorf("expected error=%v, got %v", tc.expectErr, err)
+			}
+		})
+	}
+}