From 641863358383dca9b0b49b40f23a626c6adc9d55 Mon Sep 17 00:00:00 2001 From: asouchang <2739813+asouchang@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:39:05 +0800 Subject: [PATCH 1/7] feat: add --storage-namespace flag and HELM_DIFF_STORAGE_NAMESPACE env var Support retrieving live Helm release manifests, values, and hooks from a separate storage namespace (such as flux-system) while rendering chart templates in the target namespace (-n/--namespace). - Add --storage-namespace flag to upgrade, revision, and rollback commands - Add HELM_DIFF_STORAGE_NAMESPACE environment variable support - Add unit tests covering flag parsing, env var resolution, and storage namespace fallback - Update README documentation Signed-off-by: asouchang <2739813+asouchang@users.noreply.github.com> --- README.md | 9 ++++ cmd/helm.go | 4 +- cmd/revision.go | 37 +++++++++++++---- cmd/revision_test.go | 97 +++++++++++++++++++++++++++++++++++++++++++ cmd/rollback.go | 29 ++++++++++--- cmd/rollback_test.go | 97 +++++++++++++++++++++++++++++++++++++++++++ cmd/upgrade.go | 31 +++++++++++--- cmd/upgrade_test.go | 99 ++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 382 insertions(+), 21 deletions(-) create mode 100644 cmd/revision_test.go create mode 100644 cmd/rollback_test.go diff --git a/README.md b/README.md index 968b2f92..f20f2a9a 100644 --- a/README.md +++ b/README.md @@ -178,6 +178,7 @@ Flags: --show-secrets do not redact secret values in the output --show-secrets-decoded decode secret values in the output --skip-schema-validation skip validation of the rendered manifests against the Kubernetes OpenAPI schema + --storage-namespace string namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace) --strip-trailing-cr strip trailing carriage return on input --suppress stringArray allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service') --suppress-output-line-regex stringArray a regex to suppress diff output lines that match @@ -323,6 +324,11 @@ Examples: # Read the flag usage below for more information on --context. HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog + # Set HELM_DIFF_STORAGE_NAMESPACE=flux-system to + # fetch release manifests/values/hooks from a storage namespace different from the target namespace. + # This is equivalent to specifying the --storage-namespace flag. + HELM_DIFF_STORAGE_NAMESPACE=flux-system helm diff upgrade -n prod-apps my-release datadog/datadog + Flags: --allow-unreleased enables diffing of releases that are not yet deployed via Helm -a, --api-versions stringArray Kubernetes api versions used for Capabilities.APIVersions @@ -360,6 +366,7 @@ Flags: --show-secrets do not redact secret values in the output --show-secrets-decoded decode secret values in the output --skip-schema-validation skip validation of the rendered manifests against the Kubernetes OpenAPI schema + --storage-namespace string namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace) --strip-trailing-cr strip trailing carriage return on input --suppress stringArray allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service') --suppress-output-line-regex stringArray a regex to suppress diff output lines that match @@ -445,6 +452,7 @@ Flags: --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") --show-secrets do not redact secret values in the output --show-secrets-decoded decode secret values in the output + --storage-namespace string namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace) --strip-trailing-cr strip trailing carriage return on input --suppress stringArray allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service') --suppress-output-line-regex stringArray a regex to suppress diff output lines that match @@ -481,6 +489,7 @@ Flags: --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") --show-secrets do not redact secret values in the output --show-secrets-decoded decode secret values in the output + --storage-namespace string namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace) --strip-trailing-cr strip trailing carriage return on input --suppress stringArray allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service') --suppress-output-line-regex stringArray a regex to suppress diff output lines that match diff --git a/cmd/helm.go b/cmd/helm.go index 5a3f39df..0bf0560a 100644 --- a/cmd/helm.go +++ b/cmd/helm.go @@ -428,8 +428,8 @@ func (d *diffCmd) writeExistingValues(f *os.File, all bool) error { if all { args = append(args, "--all") } - if d.namespace != "" { - args = append(args, "--namespace", d.namespace) + if storageNs := d.getStorageNamespace(); storageNs != "" { + args = append(args, "--namespace", storageNs) } if d.kubeContext != "" { args = append(args, "--kube-context", d.kubeContext) diff --git a/cmd/revision.go b/cmd/revision.go index 081e6582..ff889351 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -14,6 +14,8 @@ import ( type revision struct { release string + namespace string + storageNamespace string kubeContext string detailedExitCode bool revisions []string @@ -22,6 +24,13 @@ type revision struct { diff.Options } +func (d *revision) getStorageNamespace() string { + if d.storageNamespace != "" { + return d.storageNamespace + } + return d.namespace +} + const revisionCmdLongUsage = ` This command compares the manifests details of a named release. @@ -60,6 +69,13 @@ func revisionCmd() *cobra.Command { return errors.New("Too many arguments to Command \"revision\".\nMaximum 3 arguments allowed: release name, revision1, revision2") } + if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { + diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + if !cmd.Flags().Changed("namespace") && diff.namespace == "" { + diff.namespace = os.Getenv("HELM_NAMESPACE") + } + ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -68,6 +84,8 @@ func revisionCmd() *cobra.Command { }, } + revisionCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") + revisionCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") revisionCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") revisionCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") revisionCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") @@ -80,27 +98,28 @@ func revisionCmd() *cobra.Command { } func (d *revision) differentiateHelm3() error { - namespace := os.Getenv("HELM_NAMESPACE") + storageNs := d.getStorageNamespace() + targetNs := d.namespace excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook} if d.includeTests { excludes = []string{} } switch len(d.revisions) { case 1: - releaseResponse, err := getRelease(d.release, 0, namespace, d.kubeContext) + releaseResponse, err := getRelease(d.release, 0, storageNs, d.kubeContext) if err != nil { return err } revision, _ := strconv.Atoi(d.revisions[0]) - revisionResponse, err := getRelease(d.release, revision, namespace, d.kubeContext) + revisionResponse, err := getRelease(d.release, revision, storageNs, d.kubeContext) if err != nil { return err } - oldSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...) - newSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...) + oldSpecs := manifest.Parse(revisionResponse, targetNs, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(releaseResponse, targetNs, d.normalizeManifests, excludes...) revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation @@ -117,18 +136,18 @@ func (d *revision) differentiateHelm3() error { revision1, revision2 = revision2, revision1 } - revisionResponse1, err := getRelease(d.release, revision1, namespace, d.kubeContext) + revisionResponse1, err := getRelease(d.release, revision1, storageNs, d.kubeContext) if err != nil { return err } - revisionResponse2, err := getRelease(d.release, revision2, namespace, d.kubeContext) + revisionResponse2, err := getRelease(d.release, revision2, storageNs, d.kubeContext) if err != nil { return err } - oldSpecs := manifest.Parse(revisionResponse1, namespace, d.normalizeManifests, excludes...) - newSpecs := manifest.Parse(revisionResponse2, namespace, d.normalizeManifests, excludes...) + oldSpecs := manifest.Parse(revisionResponse1, targetNs, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(revisionResponse2, targetNs, d.normalizeManifests, excludes...) revisionResponse1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation revisionResponse2 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation diff --git a/cmd/revision_test.go b/cmd/revision_test.go new file mode 100644 index 00000000..0732f910 --- /dev/null +++ b/cmd/revision_test.go @@ -0,0 +1,97 @@ +package cmd + +import ( + "os" + "testing" +) + +func TestRevisionCommand_StorageNamespaceFlag(t *testing.T) { + cmd := revisionCmd() + f := cmd.Flags() + + if f.Lookup("storage-namespace") == nil { + t.Fatal("expected flag --storage-namespace to be registered on revisionCmd") + } + + if f.Lookup("namespace") == nil { + t.Fatal("expected flag --namespace to be registered on revisionCmd") + } + + if f.ShorthandLookup("n") == nil { + t.Fatal("expected shorthand flag -n to be registered on revisionCmd") + } + + err := cmd.ParseFlags([]string{"--storage-namespace", "flux-system", "-n", "prod-apps"}) + if err != nil { + t.Fatalf("unexpected error parsing flags: %v", err) + } + + storageNs, err := cmd.Flags().GetString("storage-namespace") + if err != nil || storageNs != "flux-system" { + t.Errorf("expected storage-namespace=flux-system, got %q (err: %v)", storageNs, err) + } + + ns, err := cmd.Flags().GetString("namespace") + if err != nil || ns != "prod-apps" { + t.Errorf("expected namespace=prod-apps, got %q (err: %v)", ns, err) + } +} + +func TestRevision_GetStorageNamespace(t *testing.T) { + original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) + + cases := []struct { + name string + namespace string + storageNamespace string + envVar string + expected string + }{ + { + name: "defaults to target namespace", + namespace: "target-ns", + storageNamespace: "", + envVar: "", + expected: "target-ns", + }, + { + name: "storage namespace flag set", + namespace: "target-ns", + storageNamespace: "flux-system", + envVar: "", + expected: "flux-system", + }, + { + name: "storage namespace env var set", + namespace: "target-ns", + storageNamespace: "", + envVar: "flux-system-env", + expected: "flux-system-env", + }, + { + name: "storage namespace flag overrides env var", + namespace: "target-ns", + storageNamespace: "flux-system-flag", + envVar: "flux-system-env", + expected: "flux-system-flag", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", tc.envVar) + r := revision{ + namespace: tc.namespace, + storageNamespace: tc.storageNamespace, + } + if r.storageNamespace == "" && tc.envVar != "" { + r.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + actual := r.getStorageNamespace() + if actual != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, actual) + } + }) + } +} diff --git a/cmd/rollback.go b/cmd/rollback.go index b390b125..69745dce 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -14,6 +14,8 @@ import ( type rollback struct { release string + namespace string + storageNamespace string kubeContext string detailedExitCode bool revisions []string @@ -22,6 +24,13 @@ type rollback struct { diff.Options } +func (d *rollback) getStorageNamespace() string { + if d.storageNamespace != "" { + return d.storageNamespace + } + return d.namespace +} + const rollbackCmdLongUsage = ` This command compares the latest manifest details of a named release with specific revision values to rollback. @@ -49,6 +58,13 @@ func rollbackCmd() *cobra.Command { return err } + if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { + diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + if !cmd.Flags().Changed("namespace") && diff.namespace == "" { + diff.namespace = os.Getenv("HELM_NAMESPACE") + } + ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -58,6 +74,8 @@ func rollbackCmd() *cobra.Command { }, } + rollbackCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") + rollbackCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") rollbackCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") rollbackCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") rollbackCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") @@ -70,13 +88,14 @@ func rollbackCmd() *cobra.Command { } func (d *rollback) backcastHelm3() error { - namespace := os.Getenv("HELM_NAMESPACE") + storageNs := d.getStorageNamespace() + targetNs := d.namespace excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook} if d.includeTests { excludes = []string{} } // get manifest of the latest release - releaseResponse, err := getRelease(d.release, 0, namespace, d.kubeContext) + releaseResponse, err := getRelease(d.release, 0, storageNs, d.kubeContext) if err != nil { return err @@ -84,14 +103,14 @@ func (d *rollback) backcastHelm3() error { // get manifest of the release to rollback revision, _ := strconv.Atoi(d.revisions[0]) - revisionResponse, err := getRelease(d.release, revision, namespace, d.kubeContext) + revisionResponse, err := getRelease(d.release, revision, storageNs, d.kubeContext) if err != nil { return err } // create a diff between the current manifest and the version of the manifest that a user is intended to rollback - oldSpecs := manifest.Parse(releaseResponse, namespace, d.normalizeManifests, excludes...) - newSpecs := manifest.Parse(revisionResponse, namespace, d.normalizeManifests, excludes...) + oldSpecs := manifest.Parse(releaseResponse, targetNs, d.normalizeManifests, excludes...) + newSpecs := manifest.Parse(revisionResponse, targetNs, d.normalizeManifests, excludes...) releaseResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation revisionResponse = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation diff --git a/cmd/rollback_test.go b/cmd/rollback_test.go new file mode 100644 index 00000000..b67653cf --- /dev/null +++ b/cmd/rollback_test.go @@ -0,0 +1,97 @@ +package cmd + +import ( + "os" + "testing" +) + +func TestRollbackCommand_StorageNamespaceFlag(t *testing.T) { + cmd := rollbackCmd() + f := cmd.Flags() + + if f.Lookup("storage-namespace") == nil { + t.Fatal("expected flag --storage-namespace to be registered on rollbackCmd") + } + + if f.Lookup("namespace") == nil { + t.Fatal("expected flag --namespace to be registered on rollbackCmd") + } + + if f.ShorthandLookup("n") == nil { + t.Fatal("expected shorthand flag -n to be registered on rollbackCmd") + } + + err := cmd.ParseFlags([]string{"--storage-namespace", "flux-system", "-n", "prod-apps"}) + if err != nil { + t.Fatalf("unexpected error parsing flags: %v", err) + } + + storageNs, err := cmd.Flags().GetString("storage-namespace") + if err != nil || storageNs != "flux-system" { + t.Errorf("expected storage-namespace=flux-system, got %q (err: %v)", storageNs, err) + } + + ns, err := cmd.Flags().GetString("namespace") + if err != nil || ns != "prod-apps" { + t.Errorf("expected namespace=prod-apps, got %q (err: %v)", ns, err) + } +} + +func TestRollback_GetStorageNamespace(t *testing.T) { + original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) + + cases := []struct { + name string + namespace string + storageNamespace string + envVar string + expected string + }{ + { + name: "defaults to target namespace", + namespace: "target-ns", + storageNamespace: "", + envVar: "", + expected: "target-ns", + }, + { + name: "storage namespace flag set", + namespace: "target-ns", + storageNamespace: "flux-system", + envVar: "", + expected: "flux-system", + }, + { + name: "storage namespace env var set", + namespace: "target-ns", + storageNamespace: "", + envVar: "flux-system-env", + expected: "flux-system-env", + }, + { + name: "storage namespace flag overrides env var", + namespace: "target-ns", + storageNamespace: "flux-system-flag", + envVar: "flux-system-env", + expected: "flux-system-flag", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", tc.envVar) + r := rollback{ + namespace: tc.namespace, + storageNamespace: tc.storageNamespace, + } + if r.storageNamespace == "" && tc.envVar != "" { + r.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + actual := r.getStorageNamespace() + if actual != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, actual) + } + }) + } +} diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 63c8ebdb..602b2fca 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -48,6 +48,7 @@ type diffCmd struct { enableDNS bool SkipSchemaValidation bool namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace. + storageNamespace string // namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to namespace. valueFiles valueFiles values []string stringValues []string @@ -92,6 +93,13 @@ func (d *diffCmd) isAllowUnreleased() bool { return d.allowUnreleased || d.install } +func (d *diffCmd) getStorageNamespace() string { + if d.storageNamespace != "" { + return d.storageNamespace + } + return d.namespace +} + // clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree. // // helm-diff basically have 2 modes of operation: @@ -227,6 +235,13 @@ func newChartCommand() *cobra.Command { } } + if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { + diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + if !cmd.Flags().Changed("namespace") && diff.namespace == "" { + diff.namespace = os.Getenv("HELM_NAMESPACE") + } + ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -241,6 +256,8 @@ func newChartCommand() *cobra.Command { f := cmd.Flags() var kubeconfig string f.StringVar(&kubeconfig, "kubeconfig", "", "This flag is ignored, to allow passing of this top level flag to helm") + f.StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") + f.StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") f.BoolVar(&diff.threeWayMerge, "three-way-merge", false, "use three-way-merge to compute patch and generate diff output") f.StringVar(&diff.kubeContext, "kube-context", "", "name of the kubeconfig context to use") f.StringVar(&diff.chartVersion, "version", "", "specify the exact chart version to use. If this is not specified, the latest version is used") @@ -303,13 +320,13 @@ func (d *diffCmd) runHelm3() error { } if d.clusterAccessAllowed() { - releaseManifest, err = getRelease(d.release, d.revision, d.namespace, d.kubeContext) + releaseManifest, err = getRelease(d.release, d.revision, d.getStorageNamespace(), 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) + return fmt.Errorf("Failed to get revision %d of release %s in namespace %s: %w", d.revision, d.release, d.getStorageNamespace(), err) } if d.isAllowUnreleased() { newInstall = true @@ -320,7 +337,7 @@ func (d *diffCmd) runHelm3() error { } } if err != nil { - return fmt.Errorf("Failed to get release %s in namespace %s: %w", d.release, d.namespace, err) + return fmt.Errorf("Failed to get release %s in namespace %s: %w", d.release, d.getStorageNamespace(), err) } installManifest, err := d.template(!newInstall) @@ -332,7 +349,11 @@ func (d *diffCmd) runHelm3() error { if d.threeWayMerge || d.takeOwnership { actionConfig = new(action.Configuration) localEnv := prepareEnvSettings(d.kubeContext) - if err := actionConfig.Init(localEnv.RESTClientGetter(), localEnv.Namespace(), os.Getenv("HELM_DRIVER")); err != nil { + storageNs := d.getStorageNamespace() + if storageNs == "" { + storageNs = localEnv.Namespace() + } + if err := actionConfig.Init(localEnv.RESTClientGetter(), storageNs, os.Getenv("HELM_DRIVER")); err != nil { log.Fatalf("%+v", err) } if err := actionConfig.KubeClient.IsReachable(); err != nil { @@ -350,7 +371,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.revision, d.namespace, d.kubeContext) + hooks, err := getHooks(d.release, d.revision, d.getStorageNamespace(), d.kubeContext) if err != nil { return err } diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 60744a54..4ffbced0 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -233,3 +233,102 @@ func TestValidateRevision(t *testing.T) { }) } } + +func TestGetStorageNamespace(t *testing.T) { + cases := []struct { + name string + namespace string + storageNamespace string + expected string + }{ + { + name: "storage namespace defaults to target namespace when unset", + namespace: "target-ns", + storageNamespace: "", + expected: "target-ns", + }, + { + name: "storage namespace overrides target namespace when set", + namespace: "target-ns", + storageNamespace: "flux-system", + expected: "flux-system", + }, + { + name: "both empty returns empty", + namespace: "", + storageNamespace: "", + expected: "", + }, + { + name: "storage namespace set with empty target namespace", + namespace: "", + storageNamespace: "flux-system", + expected: "flux-system", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d := diffCmd{ + namespace: tc.namespace, + storageNamespace: tc.storageNamespace, + } + actual := d.getStorageNamespace() + if actual != tc.expected { + t.Errorf("expected %q, got %q", tc.expected, actual) + } + }) + } +} + +func TestUpgradeCommand_StorageNamespaceFlag(t *testing.T) { + cmd := newChartCommand() + f := cmd.Flags() + + if f.Lookup("storage-namespace") == nil { + t.Fatal("expected flag --storage-namespace to be registered") + } + + if f.Lookup("namespace") == nil { + t.Fatal("expected flag --namespace to be registered") + } + + if f.ShorthandLookup("n") == nil { + t.Fatal("expected shorthand flag -n to be registered") + } + + err := cmd.ParseFlags([]string{"--storage-namespace", "flux-system", "-n", "prod-apps"}) + if err != nil { + t.Fatalf("unexpected error parsing flags: %v", err) + } + + storageNs, err := cmd.Flags().GetString("storage-namespace") + if err != nil || storageNs != "flux-system" { + t.Errorf("expected storage-namespace=flux-system, got %q (err: %v)", storageNs, err) + } + + ns, err := cmd.Flags().GetString("namespace") + if err != nil || ns != "prod-apps" { + t.Errorf("expected namespace=prod-apps, got %q (err: %v)", ns, err) + } +} + +func TestUpgradeCommand_StorageNamespaceEnvVar(t *testing.T) { + original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) + + os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "flux-system-env") + + // When flag is not specified, RunE or initialization logic should pick up env var + cmd := newChartCommand() + _ = cmd.ParseFlags([]string{}) + + // Check resolution logic with env var + d := diffCmd{ + namespace: "my-target-ns", + storageNamespace: os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), + } + if d.getStorageNamespace() != "flux-system-env" { + t.Errorf("expected getStorageNamespace() to return env var %q, got %q", "flux-system-env", d.getStorageNamespace()) + } +} From ef5e6791bf5d475f249e016822a4ddb28a771d58 Mon Sep 17 00:00:00 2001 From: asouchang <2739813+asouchang@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:04:57 +0800 Subject: [PATCH 2/7] refactor: deduplicate namespace resolution and add end-to-end fake helm tests Address review feedback: - Extract shared resolveStorageNamespace and resolveNamespaceFlags helpers to cmd/helpers.go - Deduplicate storage namespace and env var resolution across upgrade, revision, and rollback - Add comprehensive end-to-end command execution tests using fake Helm (capture_args mode) verifying helm get and helm template namespace separation - Add unit tests for shared namespace helper functions Signed-off-by: asouchang <2739813+asouchang@users.noreply.github.com> --- cmd/helpers.go | 20 +++++++ cmd/helpers_test.go | 65 ++++++++++++++++++++ cmd/main_test.go | 27 +++++++-- cmd/revision.go | 12 +--- cmd/revision_test.go | 140 ++++++++++++++++++++++++++----------------- cmd/rollback.go | 12 +--- cmd/rollback_test.go | 140 ++++++++++++++++++++++++++----------------- cmd/upgrade.go | 12 +--- cmd/upgrade_test.go | 112 +++++++++++++++++++++++++++++----- 9 files changed, 378 insertions(+), 162 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index fc56a8ef..4711670c 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "github.com/spf13/cobra" "k8s.io/client-go/util/homedir" ) @@ -34,3 +35,22 @@ func outputWithRichError(cmd *exec.Cmd) ([]byte, error) { } return output, err } + +// resolveStorageNamespace returns storageNamespace if non-empty, otherwise falls back to namespace. +func resolveStorageNamespace(storageNamespace, namespace string) string { + if storageNamespace != "" { + return storageNamespace + } + return namespace +} + +// resolveNamespaceFlags populates storageNamespace and namespace from their respective environment variables +// (HELM_DIFF_STORAGE_NAMESPACE and HELM_NAMESPACE) if the flags were not explicitly set on the command line. +func resolveNamespaceFlags(cmd *cobra.Command, storageNamespace, namespace *string) { + if !cmd.Flags().Changed("storage-namespace") && *storageNamespace == "" { + *storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") + } + if !cmd.Flags().Changed("namespace") && *namespace == "" { + *namespace = os.Getenv("HELM_NAMESPACE") + } +} diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index a0eb1e72..2a7cf32b 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -140,3 +140,68 @@ func TestOutputWithRichError(t *testing.T) { }) } } + +func TestResolveStorageNamespace(t *testing.T) { + cases := []struct { + name string + storageNamespace string + namespace string + expected string + }{ + { + name: "storage namespace set returns storage namespace", + storageNamespace: "flux-system", + namespace: "prod-apps", + expected: "flux-system", + }, + { + name: "storage namespace empty returns target namespace", + storageNamespace: "", + namespace: "prod-apps", + expected: "prod-apps", + }, + { + name: "both empty returns empty", + storageNamespace: "", + namespace: "", + expected: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := resolveStorageNamespace(tc.storageNamespace, tc.namespace) + require.Equal(t, tc.expected, actual) + }) + } +} + +func TestResolveNamespaceFlags(t *testing.T) { + t.Run("env vars populate unset flags", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") + t.Setenv("HELM_NAMESPACE", "env-target") + + cmd := newChartCommand() + var storageNs, ns string + resolveNamespaceFlags(cmd, &storageNs, &ns) + + require.Equal(t, "env-storage", storageNs) + require.Equal(t, "env-target", ns) + }) + + t.Run("explicit flags take precedence over env vars", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") + t.Setenv("HELM_NAMESPACE", "env-target") + + cmd := newChartCommand() + err := cmd.ParseFlags([]string{"--storage-namespace", "flag-storage", "-n", "flag-target"}) + require.NoError(t, err) + + storageNs, _ := cmd.Flags().GetString("storage-namespace") + ns, _ := cmd.Flags().GetString("namespace") + resolveNamespaceFlags(cmd, &storageNs, &ns) + + require.Equal(t, "flag-storage", storageNs) + require.Equal(t, "flag-target", ns) + }) +} diff --git a/cmd/main_test.go b/cmd/main_test.go index 813a9b3c..c9b2e252 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -51,14 +51,31 @@ func TestMain(m *testing.M) { case "capture_args": argsFile := os.Getenv("HELM_DIFF_FAKE_ARGS_FILE") if argsFile != "" { - if err := os.WriteFile(argsFile, []byte(strings.Join(os.Args[1:], " ")), 0644); err != nil { - fmt.Fprintf(os.Stderr, "failed to write fake helm args file %q: %v\n", argsFile, err) - os.Exit(1) + f, err := os.OpenFile(argsFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) + if err == nil { + _, _ = fmt.Fprintln(f, strings.Join(os.Args[1:], " ")) + _ = f.Close() + } + } + if len(os.Args) > 1 && os.Args[1] == "version" { + if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { + fmt.Print(v) + } else { + fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) } + } else { + fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) } - fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) default: - fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) + if len(os.Args) > 1 && os.Args[1] == "version" { + if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { + fmt.Print(v) + } else { + fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) + } + } else { + fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) + } } os.Exit(0) } diff --git a/cmd/revision.go b/cmd/revision.go index ff889351..6e82589a 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -25,10 +25,7 @@ type revision struct { } func (d *revision) getStorageNamespace() string { - if d.storageNamespace != "" { - return d.storageNamespace - } - return d.namespace + return resolveStorageNamespace(d.storageNamespace, d.namespace) } const revisionCmdLongUsage = ` @@ -69,12 +66,7 @@ func revisionCmd() *cobra.Command { return errors.New("Too many arguments to Command \"revision\".\nMaximum 3 arguments allowed: release name, revision1, revision2") } - if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { - diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - if !cmd.Flags().Changed("namespace") && diff.namespace == "" { - diff.namespace = os.Getenv("HELM_NAMESPACE") - } + resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) ProcessDiffOptions(cmd.Flags(), &diff.Options) diff --git a/cmd/revision_test.go b/cmd/revision_test.go index 0732f910..0962c871 100644 --- a/cmd/revision_test.go +++ b/cmd/revision_test.go @@ -2,6 +2,7 @@ package cmd import ( "os" + "strings" "testing" ) @@ -37,61 +38,88 @@ func TestRevisionCommand_StorageNamespaceFlag(t *testing.T) { } } -func TestRevision_GetStorageNamespace(t *testing.T) { - original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) - - cases := []struct { - name string - namespace string - storageNamespace string - envVar string - expected string - }{ - { - name: "defaults to target namespace", - namespace: "target-ns", - storageNamespace: "", - envVar: "", - expected: "target-ns", - }, - { - name: "storage namespace flag set", - namespace: "target-ns", - storageNamespace: "flux-system", - envVar: "", - expected: "flux-system", - }, - { - name: "storage namespace env var set", - namespace: "target-ns", - storageNamespace: "", - envVar: "flux-system-env", - expected: "flux-system-env", - }, - { - name: "storage namespace flag overrides env var", - namespace: "target-ns", - storageNamespace: "flux-system-flag", - envVar: "flux-system-env", - expected: "flux-system-flag", - }, - } +func TestRevisionCommand_Execution_StorageNamespace(t *testing.T) { + manifestYAML := `--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-config + namespace: prod-apps +data: + key: value +` - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", tc.envVar) - r := revision{ - namespace: tc.namespace, - storageNamespace: tc.storageNamespace, - } - if r.storageNamespace == "" && tc.envVar != "" { - r.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - actual := r.getStorageNamespace() - if actual != tc.expected { - t.Errorf("expected %q, got %q", tc.expected, actual) - } - }) - } + t.Run("explicit flag passes storage namespace to helm get", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + + cmd := revisionCmd() + cmd.SetArgs([]string{"my-release", "1", "2", "--storage-namespace", "flux-system", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing revision command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --revision 1 --namespace flux-system") { + t.Errorf("expected 'helm get manifest' for revision 1 to use --namespace flux-system, got:\n%s", argsContent) + } + if !strings.Contains(argsContent, "get manifest my-release --revision 2 --namespace flux-system") { + t.Errorf("expected 'helm get manifest' for revision 2 to use --namespace flux-system, got:\n%s", argsContent) + } + }) + + t.Run("env var sets storage namespace when flag is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "flux-system-env") + + cmd := revisionCmd() + cmd.SetArgs([]string{"my-release", "1", "2", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing revision command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --revision 1 --namespace flux-system-env") { + t.Errorf("expected 'helm get manifest' to use env var --namespace flux-system-env, got:\n%s", argsContent) + } + }) + + t.Run("defaults to target namespace when storage namespace is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") + + cmd := revisionCmd() + cmd.SetArgs([]string{"my-release", "1", "2", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing revision command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --revision 1 --namespace prod-apps") { + t.Errorf("expected 'helm get manifest' to fall back to target namespace --namespace prod-apps, got:\n%s", argsContent) + } + }) } diff --git a/cmd/rollback.go b/cmd/rollback.go index 69745dce..6577e202 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -25,10 +25,7 @@ type rollback struct { } func (d *rollback) getStorageNamespace() string { - if d.storageNamespace != "" { - return d.storageNamespace - } - return d.namespace + return resolveStorageNamespace(d.storageNamespace, d.namespace) } const rollbackCmdLongUsage = ` @@ -58,12 +55,7 @@ func rollbackCmd() *cobra.Command { return err } - if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { - diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - if !cmd.Flags().Changed("namespace") && diff.namespace == "" { - diff.namespace = os.Getenv("HELM_NAMESPACE") - } + resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) ProcessDiffOptions(cmd.Flags(), &diff.Options) diff --git a/cmd/rollback_test.go b/cmd/rollback_test.go index b67653cf..89d45abf 100644 --- a/cmd/rollback_test.go +++ b/cmd/rollback_test.go @@ -2,6 +2,7 @@ package cmd import ( "os" + "strings" "testing" ) @@ -37,61 +38,88 @@ func TestRollbackCommand_StorageNamespaceFlag(t *testing.T) { } } -func TestRollback_GetStorageNamespace(t *testing.T) { - original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) - - cases := []struct { - name string - namespace string - storageNamespace string - envVar string - expected string - }{ - { - name: "defaults to target namespace", - namespace: "target-ns", - storageNamespace: "", - envVar: "", - expected: "target-ns", - }, - { - name: "storage namespace flag set", - namespace: "target-ns", - storageNamespace: "flux-system", - envVar: "", - expected: "flux-system", - }, - { - name: "storage namespace env var set", - namespace: "target-ns", - storageNamespace: "", - envVar: "flux-system-env", - expected: "flux-system-env", - }, - { - name: "storage namespace flag overrides env var", - namespace: "target-ns", - storageNamespace: "flux-system-flag", - envVar: "flux-system-env", - expected: "flux-system-flag", - }, - } +func TestRollbackCommand_Execution_StorageNamespace(t *testing.T) { + manifestYAML := `--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-config + namespace: prod-apps +data: + key: value +` - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", tc.envVar) - r := rollback{ - namespace: tc.namespace, - storageNamespace: tc.storageNamespace, - } - if r.storageNamespace == "" && tc.envVar != "" { - r.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - actual := r.getStorageNamespace() - if actual != tc.expected { - t.Errorf("expected %q, got %q", tc.expected, actual) - } - }) - } + t.Run("explicit flag passes storage namespace to helm get", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + + cmd := rollbackCmd() + cmd.SetArgs([]string{"my-release", "2", "--storage-namespace", "flux-system", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing rollback command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --namespace flux-system") { + t.Errorf("expected 'helm get manifest' for latest release to use --namespace flux-system, got:\n%s", argsContent) + } + if !strings.Contains(argsContent, "get manifest my-release --revision 2 --namespace flux-system") { + t.Errorf("expected 'helm get manifest' for revision 2 to use --namespace flux-system, got:\n%s", argsContent) + } + }) + + t.Run("env var sets storage namespace when flag is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "flux-system-env") + + cmd := rollbackCmd() + cmd.SetArgs([]string{"my-release", "2", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing rollback command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --namespace flux-system-env") { + t.Errorf("expected 'helm get manifest' to use env var --namespace flux-system-env, got:\n%s", argsContent) + } + }) + + t.Run("defaults to target namespace when storage namespace is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") + + cmd := rollbackCmd() + cmd.SetArgs([]string{"my-release", "2", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing rollback command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + if !strings.Contains(argsContent, "get manifest my-release --namespace prod-apps") { + t.Errorf("expected 'helm get manifest' to fall back to target namespace --namespace prod-apps, got:\n%s", argsContent) + } + }) } diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 602b2fca..779a4717 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -94,10 +94,7 @@ func (d *diffCmd) isAllowUnreleased() bool { } func (d *diffCmd) getStorageNamespace() string { - if d.storageNamespace != "" { - return d.storageNamespace - } - return d.namespace + return resolveStorageNamespace(d.storageNamespace, d.namespace) } // clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree. @@ -235,12 +232,7 @@ func newChartCommand() *cobra.Command { } } - if !cmd.Flags().Changed("storage-namespace") && diff.storageNamespace == "" { - diff.storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - if !cmd.Flags().Changed("namespace") && diff.namespace == "" { - diff.namespace = os.Getenv("HELM_NAMESPACE") - } + resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) ProcessDiffOptions(cmd.Flags(), &diff.Options) diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 4ffbced0..e45e8b6e 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "slices" + "strings" "testing" ) @@ -313,22 +314,103 @@ func TestUpgradeCommand_StorageNamespaceFlag(t *testing.T) { } } -func TestUpgradeCommand_StorageNamespaceEnvVar(t *testing.T) { - original := os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - defer os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", original) +func TestUpgradeCommand_Execution_StorageNamespace(t *testing.T) { + manifestYAML := `--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-config + namespace: prod-apps +data: + key: value +` + + t.Run("explicit flag separates storage and target namespace", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + + chartDir := t.TempDir() + cmd := newChartCommand() + cmd.SetArgs([]string{"my-release", chartDir, "--storage-namespace", "flux-system", "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing upgrade command: %v", err) + } - os.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "flux-system-env") + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) - // When flag is not specified, RunE or initialization logic should pick up env var - cmd := newChartCommand() - _ = cmd.ParseFlags([]string{}) + // get manifest should use storage namespace + if !strings.Contains(argsContent, "get manifest my-release --namespace flux-system") { + t.Errorf("expected 'helm get manifest' to use --namespace flux-system, got:\n%s", argsContent) + } + // template should use target namespace + if !strings.Contains(argsContent, "template my-release "+chartDir+" --namespace prod-apps") { + t.Errorf("expected 'helm template' to use --namespace prod-apps, got:\n%s", argsContent) + } + }) - // Check resolution logic with env var - d := diffCmd{ - namespace: "my-target-ns", - storageNamespace: os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), - } - if d.getStorageNamespace() != "flux-system-env" { - t.Errorf("expected getStorageNamespace() to return env var %q, got %q", "flux-system-env", d.getStorageNamespace()) - } + t.Run("env var sets storage namespace when flag is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "flux-system-env") + + chartDir := t.TempDir() + cmd := newChartCommand() + cmd.SetArgs([]string{"my-release", chartDir, "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing upgrade command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + // get manifest should use storage namespace from env var + if !strings.Contains(argsContent, "get manifest my-release --namespace flux-system-env") { + t.Errorf("expected 'helm get manifest' to use --namespace flux-system-env, got:\n%s", argsContent) + } + // template should use target namespace + if !strings.Contains(argsContent, "template my-release "+chartDir+" --namespace prod-apps") { + t.Errorf("expected 'helm template' to use --namespace prod-apps, got:\n%s", argsContent) + } + }) + + t.Run("defaults to target namespace when storage namespace is omitted", func(t *testing.T) { + argsFile := t.TempDir() + "/args" + setupFakeHelm(t, "capture_args", manifestYAML, argsFile, "") + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") + + chartDir := t.TempDir() + cmd := newChartCommand() + cmd.SetArgs([]string{"my-release", chartDir, "-n", "prod-apps"}) + + err := cmd.Execute() + if err != nil { + t.Fatalf("unexpected error executing upgrade command: %v", err) + } + + data, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("failed to read fake helm args: %v", err) + } + argsContent := string(data) + + // get manifest should use target namespace as fallback + if !strings.Contains(argsContent, "get manifest my-release --namespace prod-apps") { + t.Errorf("expected 'helm get manifest' to fall back to --namespace prod-apps, got:\n%s", argsContent) + } + // template should use target namespace + if !strings.Contains(argsContent, "template my-release "+chartDir+" --namespace prod-apps") { + t.Errorf("expected 'helm template' to use --namespace prod-apps, got:\n%s", argsContent) + } + }) } From 0ecb85466869ebc078babc5235179736fe0f34e8 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 30 Aug 2026 07:41:46 +0800 Subject: [PATCH 3/7] refactor: address review feedback on storage namespace Address the review comments on the storage namespace feature: - Simplify env var resolution: register --storage-namespace with os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") as the flag default (matching how --namespace already defaults to HELM_NAMESPACE) and drop the resolveNamespaceFlags helper, which duplicated resolution logic that was already handled by the flag defaults - Extract the duplicated fake helm version-output handling in main_test.go into printFakeHelmOutput and add TestFakeHelmVersionOutput covering HELM_DIFF_FAKE_VERSION_OUTPUT - Regenerate the README flag tables from actual --help output so the newly added -n/--namespace flag (and previously missing --kube-context rows) are listed for upgrade, revision and rollback - Document that HELM_DIFF_USE_UPGRADE_DRY_RUN=true does not support the storage/target namespace separation Signed-off-by: yxxhero --- README.md | 17 +++++++++-- cmd/helpers.go | 12 -------- cmd/helpers_test.go | 47 ++++++++++++++++++++++------ cmd/main_test.go | 74 ++++++++++++++++++++++++++++++++++----------- cmd/revision.go | 4 +-- cmd/rollback.go | 4 +-- cmd/upgrade.go | 4 +-- 7 files changed, 111 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index f20f2a9a..7658efaf 100644 --- a/README.md +++ b/README.md @@ -156,8 +156,10 @@ Flags: --include-tests enable the diffing of the helm test hooks --insecure-skip-tls-verify skip tls certificate checks for the chart download --install enables diffing of releases that are not yet deployed via Helm (equivalent to --allow-unreleased, added to match "helm upgrade --install" command + --kube-context string name of the kubeconfig context to use --kube-version string Kubernetes version used for Capabilities.KubeVersion --kubeconfig string This flag is ignored, to allow passing of this top level flag to helm + -n, --namespace string namespace to assume the release to be installed into. Defaults to the current kube config namespace. --no-color remove colors from the output. If both --no-color and --color are unspecified, coloring enabled only when the stdout is a term and TERM is not "dumb" --no-hooks disable diffing of hooks --normalize-manifests normalize manifests before running diff to exclude style differences from the output @@ -188,7 +190,7 @@ Flags: -f, --values valueFiles specify values in a YAML file (can specify multiple) (default []) --version string specify the exact chart version to use. If this is not specified, the latest version is used -Additional help topcis: +Additional help topics: diff Use "diff [command] --help" for more information about a command. @@ -329,6 +331,12 @@ HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog # This is equivalent to specifying the --storage-namespace flag. HELM_DIFF_STORAGE_NAMESPACE=flux-system helm diff upgrade -n prod-apps my-release datadog/datadog + # NOTE: The storage namespace separation is not supported in combination with + # HELM_DIFF_USE_UPGRADE_DRY_RUN=true, because rendering then goes through + # `helm upgrade --dry-run`, which resolves the release storage in the target + # namespace. If the release exists only in the storage namespace, keep the + # default `helm template` based rendering instead. + Flags: --allow-unreleased enables diffing of releases that are not yet deployed via Helm -a, --api-versions stringArray Kubernetes api versions used for Capabilities.APIVersions @@ -345,8 +353,10 @@ Flags: --include-tests enable the diffing of the helm test hooks --insecure-skip-tls-verify skip tls certificate checks for the chart download --install enables diffing of releases that are not yet deployed via Helm (equivalent to --allow-unreleased, added to match "helm upgrade --install" command + --kube-context string name of the kubeconfig context to use --kube-version string Kubernetes version used for Capabilities.KubeVersion --kubeconfig string This flag is ignored, to allow passing of this top level flag to helm + -n, --namespace string namespace to assume the release to be installed into. Defaults to the current kube config namespace. --no-hooks disable diffing of hooks --normalize-manifests normalize manifests before running diff to exclude style differences from the output --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") @@ -443,11 +453,12 @@ Usage: Flags: -C, --context int output NUM lines of context around changes (default -1) - --show-secrets-decoded decode secret values in the output --detailed-exitcode return a non-zero exit code when there are changes -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for revision --include-tests enable the diffing of the helm test hooks + --kube-context string name of the kubeconfig context to use + -n, --namespace string namespace to assume the release to be installed into. Defaults to the current kube config namespace. --normalize-manifests normalize manifests before running diff to exclude style differences from the output --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") --show-secrets do not redact secret values in the output @@ -485,6 +496,8 @@ Flags: -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for rollback --include-tests enable the diffing of the helm test hooks + --kube-context string name of the kubeconfig context to use + -n, --namespace string namespace to assume the release to be installed into. Defaults to the current kube config namespace. --normalize-manifests normalize manifests before running diff to exclude style differences from the output --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") --show-secrets do not redact secret values in the output diff --git a/cmd/helpers.go b/cmd/helpers.go index 4711670c..cc5cbc22 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -8,7 +8,6 @@ import ( "path/filepath" "strings" - "github.com/spf13/cobra" "k8s.io/client-go/util/homedir" ) @@ -43,14 +42,3 @@ func resolveStorageNamespace(storageNamespace, namespace string) string { } return namespace } - -// resolveNamespaceFlags populates storageNamespace and namespace from their respective environment variables -// (HELM_DIFF_STORAGE_NAMESPACE and HELM_NAMESPACE) if the flags were not explicitly set on the command line. -func resolveNamespaceFlags(cmd *cobra.Command, storageNamespace, namespace *string) { - if !cmd.Flags().Changed("storage-namespace") && *storageNamespace == "" { - *storageNamespace = os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") - } - if !cmd.Flags().Changed("namespace") && *namespace == "" { - *namespace = os.Getenv("HELM_NAMESPACE") - } -} diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index 2a7cf32b..d1b38913 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" ) @@ -176,20 +177,32 @@ func TestResolveStorageNamespace(t *testing.T) { } } -func TestResolveNamespaceFlags(t *testing.T) { - t.Run("env vars populate unset flags", func(t *testing.T) { +func TestStorageNamespaceEnvVarDefaults(t *testing.T) { + newCommands := map[string]func() *cobra.Command{ + "upgrade": newChartCommand, + "revision": revisionCmd, + "rollback": rollbackCmd, + } + + t.Run("env vars populate flag defaults", func(t *testing.T) { t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") t.Setenv("HELM_NAMESPACE", "env-target") - cmd := newChartCommand() - var storageNs, ns string - resolveNamespaceFlags(cmd, &storageNs, &ns) + for name, newCmd := range newCommands { + t.Run(name, func(t *testing.T) { + cmd := newCmd() + storageNs, err := cmd.Flags().GetString("storage-namespace") + require.NoError(t, err) + require.Equal(t, "env-storage", storageNs) - require.Equal(t, "env-storage", storageNs) - require.Equal(t, "env-target", ns) + ns, err := cmd.Flags().GetString("namespace") + require.NoError(t, err) + require.Equal(t, "env-target", ns) + }) + } }) - t.Run("explicit flags take precedence over env vars", func(t *testing.T) { + t.Run("explicit flags take precedence over env var defaults", func(t *testing.T) { t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") t.Setenv("HELM_NAMESPACE", "env-target") @@ -199,9 +212,23 @@ func TestResolveNamespaceFlags(t *testing.T) { storageNs, _ := cmd.Flags().GetString("storage-namespace") ns, _ := cmd.Flags().GetString("namespace") - resolveNamespaceFlags(cmd, &storageNs, &ns) - require.Equal(t, "flag-storage", storageNs) require.Equal(t, "flag-target", ns) }) + + t.Run("empty env vars leave flags empty", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") + t.Setenv("HELM_NAMESPACE", "") + + for name, newCmd := range newCommands { + t.Run(name, func(t *testing.T) { + cmd := newCmd() + storageNs, _ := cmd.Flags().GetString("storage-namespace") + require.Empty(t, storageNs) + + ns, _ := cmd.Flags().GetString("namespace") + require.Empty(t, ns) + }) + } + }) } diff --git a/cmd/main_test.go b/cmd/main_test.go index c9b2e252..95ce91d9 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -3,8 +3,11 @@ package cmd import ( "fmt" "os" + "os/exec" "strings" "testing" + + "github.com/stretchr/testify/require" ) func shouldRunFakeHelm() bool { @@ -17,6 +20,23 @@ func shouldRunFakeHelm() bool { return !strings.HasPrefix(os.Args[1], "-test.") } +// printFakeHelmOutput prints the output for a fake helm invocation. +// A `helm version` call prints helm version build info, so that the version +// checks in cmd (see getHelmVersion) work against the fake helm. The version +// output can be overridden via HELM_DIFF_FAKE_VERSION_OUTPUT. +// Any other invocation prints HELM_DIFF_FAKE_OUTPUT. +func printFakeHelmOutput() { + if len(os.Args) > 1 && os.Args[1] == "version" { + if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { + fmt.Print(v) + } else { + fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) + } + } else { + fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) + } +} + func TestMain(m *testing.M) { if shouldRunFakeHelm() { mode := os.Getenv("HELM_DIFF_FAKE_HELM_MODE") @@ -57,27 +77,45 @@ func TestMain(m *testing.M) { _ = f.Close() } } - if len(os.Args) > 1 && os.Args[1] == "version" { - if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { - fmt.Print(v) - } else { - fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) - } - } else { - fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) - } + printFakeHelmOutput() default: - if len(os.Args) > 1 && os.Args[1] == "version" { - if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { - fmt.Print(v) - } else { - fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) - } - } else { - fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) - } + printFakeHelmOutput() } os.Exit(0) } os.Exit(m.Run()) } + +func TestFakeHelmVersionOutput(t *testing.T) { + exe, err := os.Executable() + require.NoError(t, err) + + t.Run("custom version output via HELM_DIFF_FAKE_VERSION_OUTPUT", func(t *testing.T) { + t.Setenv("HELM_DIFF_FAKE_HELM", "1") + t.Setenv("HELM_DIFF_FAKE_HELM_MODE", "default") + t.Setenv("HELM_DIFF_FAKE_VERSION_OUTPUT", `version.BuildInfo{Version:"v3.99.0"}`) + + out, err := exec.Command(exe, "version").CombinedOutput() + require.NoError(t, err) + require.Equal(t, `version.BuildInfo{Version:"v3.99.0"}`, string(out)) + }) + + t.Run("default version build info", func(t *testing.T) { + t.Setenv("HELM_DIFF_FAKE_HELM", "1") + t.Setenv("HELM_DIFF_FAKE_HELM_MODE", "capture_args") + + out, err := exec.Command(exe, "version").CombinedOutput() + require.NoError(t, err) + require.Equal(t, "version.BuildInfo{Version:\"v3.18.0\"}\n", string(out)) + }) + + t.Run("non-version invocations print HELM_DIFF_FAKE_OUTPUT", func(t *testing.T) { + t.Setenv("HELM_DIFF_FAKE_HELM", "1") + t.Setenv("HELM_DIFF_FAKE_HELM_MODE", "default") + t.Setenv("HELM_DIFF_FAKE_OUTPUT", "manifest-output") + + out, err := exec.Command(exe, "get", "manifest").CombinedOutput() + require.NoError(t, err) + require.Equal(t, "manifest-output", string(out)) + }) +} diff --git a/cmd/revision.go b/cmd/revision.go index 6e82589a..1d9f9d02 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -66,8 +66,6 @@ func revisionCmd() *cobra.Command { return errors.New("Too many arguments to Command \"revision\".\nMaximum 3 arguments allowed: release name, revision1, revision2") } - resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) - ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -77,7 +75,7 @@ func revisionCmd() *cobra.Command { } revisionCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - revisionCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + revisionCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") revisionCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") revisionCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") revisionCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") diff --git a/cmd/rollback.go b/cmd/rollback.go index 6577e202..c3edca6a 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -55,8 +55,6 @@ func rollbackCmd() *cobra.Command { return err } - resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) - ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -67,7 +65,7 @@ func rollbackCmd() *cobra.Command { } rollbackCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - rollbackCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + rollbackCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") rollbackCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") rollbackCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") rollbackCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 779a4717..5fd96210 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -232,8 +232,6 @@ func newChartCommand() *cobra.Command { } } - resolveNamespaceFlags(cmd, &diff.storageNamespace, &diff.namespace) - ProcessDiffOptions(cmd.Flags(), &diff.Options) diff.release = args[0] @@ -249,7 +247,7 @@ func newChartCommand() *cobra.Command { var kubeconfig string f.StringVar(&kubeconfig, "kubeconfig", "", "This flag is ignored, to allow passing of this top level flag to helm") f.StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - f.StringVar(&diff.storageNamespace, "storage-namespace", "", "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + f.StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") f.BoolVar(&diff.threeWayMerge, "three-way-merge", false, "use three-way-merge to compute patch and generate diff output") f.StringVar(&diff.kubeContext, "kube-context", "", "name of the kubeconfig context to use") f.StringVar(&diff.chartVersion, "version", "", "specify the exact chart version to use. If this is not specified, the latest version is used") From 43c90638749cae4f431e4618507b691d1d2e22ba Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 30 Aug 2026 07:56:07 +0800 Subject: [PATCH 4/7] refactor: deduplicate namespace flags and regenerate README via make readme Maintainability and readability improvements from the deep review: - Introduce a shared namespaces struct holding the target and storage namespace, with the storage() fallback method and a single addNamespaceFlags helper registering -n/--namespace and --storage-namespace (including flag help texts and env var defaults). Embed it into diffCmd, revision and rollback, removing the three copies of the getStorageNamespace method and the three copies of the flag registration boilerplate - Remove the redundant diffCmd namespace initialization in newChartCommand: the flag default already assigns the same HELM_NAMESPACE value at registration time - Add scripts/gen-readme.sh plus make readme / make verify-readme to regenerate the cobra flag tables in README.md from the actual --help output, and verify them in CI, so the documented flags can no longer drift (the release table had drifted: missing --kube-context and --show-secrets-decoded rows, now fixed) Signed-off-by: yxxhero --- .github/workflows/ci.yaml | 3 + Makefile | 9 +++ README.md | 2 + cmd/helm.go | 2 +- cmd/helpers.go | 8 --- cmd/helpers_test.go | 92 -------------------------- cmd/namespaces.go | 43 ++++++++++++ cmd/namespaces_test.go | 133 ++++++++++++++++++++++++++++++++++++++ cmd/revision.go | 12 +--- cmd/rollback.go | 12 +--- cmd/upgrade.go | 24 +++---- cmd/upgrade_test.go | 47 -------------- scripts/gen-readme.sh | 77 ++++++++++++++++++++++ 13 files changed, 282 insertions(+), 182 deletions(-) create mode 100644 cmd/namespaces.go create mode 100644 cmd/namespaces_test.go create mode 100755 scripts/gen-readme.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 103c8909..f63e4934 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -21,6 +21,9 @@ jobs: - name: Run unit tests run: make test + - name: Verify README flag tables + run: make verify-readme + - name: Verify installation run: | mkdir -p helmhome diff --git a/Makefile b/Makefile index ff362c58..941d5318 100644 --- a/Makefile +++ b/Makefile @@ -41,6 +41,15 @@ test: go test -v ./... -coverprofile cover.out -race go tool cover -func cover.out +.PHONY: readme +readme: build + scripts/gen-readme.sh bin/diff + +.PHONY: verify-readme +verify-readme: build + scripts/gen-readme.sh bin/diff + git diff --exit-code README.md + .PHONY: docker-run-release docker-run-release: export pkg=/go/src/github.com/databus23/helm-diff docker-run-release: diff --git a/README.md b/README.md index 7658efaf..03310534 100644 --- a/README.md +++ b/README.md @@ -416,9 +416,11 @@ Flags: -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for release --include-tests enable the diffing of the helm test hooks + --kube-context string name of the kubeconfig context to use --normalize-manifests normalize manifests before running diff to exclude style differences from the output --output string Possible values: diff, simple, template, json, structured, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff") --show-secrets do not redact secret values in the output + --show-secrets-decoded decode secret values in the output --strip-trailing-cr strip trailing carriage return on input --suppress stringArray allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service') --suppress-output-line-regex stringArray a regex to suppress diff output lines that match diff --git a/cmd/helm.go b/cmd/helm.go index 0bf0560a..2d7f36bf 100644 --- a/cmd/helm.go +++ b/cmd/helm.go @@ -428,7 +428,7 @@ func (d *diffCmd) writeExistingValues(f *os.File, all bool) error { if all { args = append(args, "--all") } - if storageNs := d.getStorageNamespace(); storageNs != "" { + if storageNs := d.storage(); storageNs != "" { args = append(args, "--namespace", storageNs) } if d.kubeContext != "" { diff --git a/cmd/helpers.go b/cmd/helpers.go index cc5cbc22..fc56a8ef 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -34,11 +34,3 @@ func outputWithRichError(cmd *exec.Cmd) ([]byte, error) { } return output, err } - -// resolveStorageNamespace returns storageNamespace if non-empty, otherwise falls back to namespace. -func resolveStorageNamespace(storageNamespace, namespace string) string { - if storageNamespace != "" { - return storageNamespace - } - return namespace -} diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index d1b38913..a0eb1e72 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -7,7 +7,6 @@ import ( "os/exec" "testing" - "github.com/spf13/cobra" "github.com/stretchr/testify/require" ) @@ -141,94 +140,3 @@ func TestOutputWithRichError(t *testing.T) { }) } } - -func TestResolveStorageNamespace(t *testing.T) { - cases := []struct { - name string - storageNamespace string - namespace string - expected string - }{ - { - name: "storage namespace set returns storage namespace", - storageNamespace: "flux-system", - namespace: "prod-apps", - expected: "flux-system", - }, - { - name: "storage namespace empty returns target namespace", - storageNamespace: "", - namespace: "prod-apps", - expected: "prod-apps", - }, - { - name: "both empty returns empty", - storageNamespace: "", - namespace: "", - expected: "", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - actual := resolveStorageNamespace(tc.storageNamespace, tc.namespace) - require.Equal(t, tc.expected, actual) - }) - } -} - -func TestStorageNamespaceEnvVarDefaults(t *testing.T) { - newCommands := map[string]func() *cobra.Command{ - "upgrade": newChartCommand, - "revision": revisionCmd, - "rollback": rollbackCmd, - } - - t.Run("env vars populate flag defaults", func(t *testing.T) { - t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") - t.Setenv("HELM_NAMESPACE", "env-target") - - for name, newCmd := range newCommands { - t.Run(name, func(t *testing.T) { - cmd := newCmd() - storageNs, err := cmd.Flags().GetString("storage-namespace") - require.NoError(t, err) - require.Equal(t, "env-storage", storageNs) - - ns, err := cmd.Flags().GetString("namespace") - require.NoError(t, err) - require.Equal(t, "env-target", ns) - }) - } - }) - - t.Run("explicit flags take precedence over env var defaults", func(t *testing.T) { - t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") - t.Setenv("HELM_NAMESPACE", "env-target") - - cmd := newChartCommand() - err := cmd.ParseFlags([]string{"--storage-namespace", "flag-storage", "-n", "flag-target"}) - require.NoError(t, err) - - storageNs, _ := cmd.Flags().GetString("storage-namespace") - ns, _ := cmd.Flags().GetString("namespace") - require.Equal(t, "flag-storage", storageNs) - require.Equal(t, "flag-target", ns) - }) - - t.Run("empty env vars leave flags empty", func(t *testing.T) { - t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") - t.Setenv("HELM_NAMESPACE", "") - - for name, newCmd := range newCommands { - t.Run(name, func(t *testing.T) { - cmd := newCmd() - storageNs, _ := cmd.Flags().GetString("storage-namespace") - require.Empty(t, storageNs) - - ns, _ := cmd.Flags().GetString("namespace") - require.Empty(t, ns) - }) - } - }) -} diff --git a/cmd/namespaces.go b/cmd/namespaces.go new file mode 100644 index 00000000..fe5e8f20 --- /dev/null +++ b/cmd/namespaces.go @@ -0,0 +1,43 @@ +package cmd + +import ( + "os" + + "github.com/spf13/pflag" +) + +// namespaces groups the target namespace and the storage namespace of a release. +// +// The target namespace is where the release resources are (or would be) deployed +// and where chart templates are rendered into. The storage namespace is where the +// helm release storage (Secret/ConfigMap) is located. GitOps tools like the FluxCD +// HelmRelease controller persist release records in a storage namespace (e.g. +// flux-system) that differs from the target namespace of the workloads. +type namespaces struct { + namespace string // target namespace (-n/--namespace, HELM_NAMESPACE) + storageNamespace string // storage namespace (--storage-namespace, HELM_DIFF_STORAGE_NAMESPACE) +} + +// storage returns the namespace of the helm release storage, falling back to the +// target namespace when no storage namespace was configured. +func (n *namespaces) storage() string { + return resolveStorageNamespace(n.storageNamespace, n.namespace) +} + +// addNamespaceFlags registers the -n/--namespace and --storage-namespace flags on f, +// binding them to n. Both flags default to their respective environment variables +// (HELM_NAMESPACE and HELM_DIFF_STORAGE_NAMESPACE), so a flag passed on the command +// line (including an explicit empty string) always takes precedence over the +// environment. +func addNamespaceFlags(f *pflag.FlagSet, n *namespaces) { + f.StringVarP(&n.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") + f.StringVar(&n.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") +} + +// resolveStorageNamespace returns storageNamespace if non-empty, otherwise falls back to namespace. +func resolveStorageNamespace(storageNamespace, namespace string) string { + if storageNamespace != "" { + return storageNamespace + } + return namespace +} diff --git a/cmd/namespaces_test.go b/cmd/namespaces_test.go new file mode 100644 index 00000000..6367c1ea --- /dev/null +++ b/cmd/namespaces_test.go @@ -0,0 +1,133 @@ +package cmd + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +func TestNamespacesStorage(t *testing.T) { + cases := []struct { + name string + namespaces namespaces + expected string + }{ + { + name: "storage namespace defaults to target namespace when unset", + namespaces: namespaces{namespace: "target-ns"}, + expected: "target-ns", + }, + { + name: "storage namespace overrides target namespace when set", + namespaces: namespaces{namespace: "target-ns", storageNamespace: "flux-system"}, + expected: "flux-system", + }, + { + name: "both empty returns empty", + namespaces: namespaces{}, + expected: "", + }, + { + name: "storage namespace set with empty target namespace", + namespaces: namespaces{storageNamespace: "flux-system"}, + expected: "flux-system", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, tc.namespaces.storage()) + }) + } +} + +func TestResolveStorageNamespace(t *testing.T) { + cases := []struct { + name string + storageNamespace string + namespace string + expected string + }{ + { + name: "storage namespace set returns storage namespace", + storageNamespace: "flux-system", + namespace: "prod-apps", + expected: "flux-system", + }, + { + name: "storage namespace empty returns target namespace", + storageNamespace: "", + namespace: "prod-apps", + expected: "prod-apps", + }, + { + name: "both empty returns empty", + storageNamespace: "", + namespace: "", + expected: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, resolveStorageNamespace(tc.storageNamespace, tc.namespace)) + }) + } +} + +func TestStorageNamespaceEnvVarDefaults(t *testing.T) { + newCommands := map[string]func() *cobra.Command{ + "upgrade": newChartCommand, + "revision": revisionCmd, + "rollback": rollbackCmd, + } + + t.Run("env vars populate flag defaults", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") + t.Setenv("HELM_NAMESPACE", "env-target") + + for name, newCmd := range newCommands { + t.Run(name, func(t *testing.T) { + cmd := newCmd() + storageNs, err := cmd.Flags().GetString("storage-namespace") + require.NoError(t, err) + require.Equal(t, "env-storage", storageNs) + + ns, err := cmd.Flags().GetString("namespace") + require.NoError(t, err) + require.Equal(t, "env-target", ns) + }) + } + }) + + t.Run("explicit flags take precedence over env var defaults", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "env-storage") + t.Setenv("HELM_NAMESPACE", "env-target") + + cmd := newChartCommand() + err := cmd.ParseFlags([]string{"--storage-namespace", "flag-storage", "-n", "flag-target"}) + require.NoError(t, err) + + storageNs, _ := cmd.Flags().GetString("storage-namespace") + ns, _ := cmd.Flags().GetString("namespace") + require.Equal(t, "flag-storage", storageNs) + require.Equal(t, "flag-target", ns) + }) + + t.Run("empty env vars leave flags empty", func(t *testing.T) { + t.Setenv("HELM_DIFF_STORAGE_NAMESPACE", "") + t.Setenv("HELM_NAMESPACE", "") + + for name, newCmd := range newCommands { + t.Run(name, func(t *testing.T) { + cmd := newCmd() + storageNs, _ := cmd.Flags().GetString("storage-namespace") + require.Empty(t, storageNs) + + ns, _ := cmd.Flags().GetString("namespace") + require.Empty(t, ns) + }) + } + }) +} diff --git a/cmd/revision.go b/cmd/revision.go index 1d9f9d02..87a5adfb 100644 --- a/cmd/revision.go +++ b/cmd/revision.go @@ -14,8 +14,7 @@ import ( type revision struct { release string - namespace string - storageNamespace string + namespaces // target namespace (-n/--namespace) and helm release storage namespace (--storage-namespace) kubeContext string detailedExitCode bool revisions []string @@ -24,10 +23,6 @@ type revision struct { diff.Options } -func (d *revision) getStorageNamespace() string { - return resolveStorageNamespace(d.storageNamespace, d.namespace) -} - const revisionCmdLongUsage = ` This command compares the manifests details of a named release. @@ -74,8 +69,7 @@ func revisionCmd() *cobra.Command { }, } - revisionCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - revisionCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + addNamespaceFlags(revisionCmd.Flags(), &diff.namespaces) revisionCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") revisionCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") revisionCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") @@ -88,7 +82,7 @@ func revisionCmd() *cobra.Command { } func (d *revision) differentiateHelm3() error { - storageNs := d.getStorageNamespace() + storageNs := d.storage() targetNs := d.namespace excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook} if d.includeTests { diff --git a/cmd/rollback.go b/cmd/rollback.go index c3edca6a..8d6aae64 100644 --- a/cmd/rollback.go +++ b/cmd/rollback.go @@ -14,8 +14,7 @@ import ( type rollback struct { release string - namespace string - storageNamespace string + namespaces // target namespace (-n/--namespace) and helm release storage namespace (--storage-namespace) kubeContext string detailedExitCode bool revisions []string @@ -24,10 +23,6 @@ type rollback struct { diff.Options } -func (d *rollback) getStorageNamespace() string { - return resolveStorageNamespace(d.storageNamespace, d.namespace) -} - const rollbackCmdLongUsage = ` This command compares the latest manifest details of a named release with specific revision values to rollback. @@ -64,8 +59,7 @@ func rollbackCmd() *cobra.Command { }, } - rollbackCmd.Flags().StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - rollbackCmd.Flags().StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + addNamespaceFlags(rollbackCmd.Flags(), &diff.namespaces) rollbackCmd.Flags().BoolVar(&diff.detailedExitCode, "detailed-exitcode", false, "return a non-zero exit code when there are changes") rollbackCmd.Flags().BoolVar(&diff.includeTests, "include-tests", false, "enable the diffing of the helm test hooks") rollbackCmd.Flags().BoolVar(&diff.normalizeManifests, "normalize-manifests", false, "normalize manifests before running diff to exclude style differences from the output") @@ -78,7 +72,7 @@ func rollbackCmd() *cobra.Command { } func (d *rollback) backcastHelm3() error { - storageNs := d.getStorageNamespace() + storageNs := d.storage() targetNs := d.namespace excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook} if d.includeTests { diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 5fd96210..e47e1933 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -47,8 +47,7 @@ type diffCmd struct { disableOpenAPIValidation bool enableDNS bool SkipSchemaValidation bool - namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace. - storageNamespace string // namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to namespace. + namespaces // target namespace (-n/--namespace) and helm release storage namespace (--storage-namespace) valueFiles valueFiles values []string stringValues []string @@ -93,10 +92,6 @@ func (d *diffCmd) isAllowUnreleased() bool { return d.allowUnreleased || d.install } -func (d *diffCmd) getStorageNamespace() string { - return resolveStorageNamespace(d.storageNamespace, d.namespace) -} - // clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree. // // helm-diff basically have 2 modes of operation: @@ -141,9 +136,7 @@ perform. ` func newChartCommand() *cobra.Command { - diff := diffCmd{ - namespace: os.Getenv("HELM_NAMESPACE"), - } + diff := diffCmd{} unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == envTrue cmd := &cobra.Command{ @@ -246,8 +239,7 @@ func newChartCommand() *cobra.Command { f := cmd.Flags() var kubeconfig string f.StringVar(&kubeconfig, "kubeconfig", "", "This flag is ignored, to allow passing of this top level flag to helm") - f.StringVarP(&diff.namespace, "namespace", "n", os.Getenv("HELM_NAMESPACE"), "namespace to assume the release to be installed into. Defaults to the current kube config namespace.") - f.StringVar(&diff.storageNamespace, "storage-namespace", os.Getenv("HELM_DIFF_STORAGE_NAMESPACE"), "namespace where the helm release storage (Secret/ConfigMap) is located. Defaults to the target namespace (-n/--namespace)") + addNamespaceFlags(f, &diff.namespaces) f.BoolVar(&diff.threeWayMerge, "three-way-merge", false, "use three-way-merge to compute patch and generate diff output") f.StringVar(&diff.kubeContext, "kube-context", "", "name of the kubeconfig context to use") f.StringVar(&diff.chartVersion, "version", "", "specify the exact chart version to use. If this is not specified, the latest version is used") @@ -310,13 +302,13 @@ func (d *diffCmd) runHelm3() error { } if d.clusterAccessAllowed() { - releaseManifest, err = getRelease(d.release, d.revision, d.getStorageNamespace(), d.kubeContext) + releaseManifest, err = getRelease(d.release, d.revision, d.storage(), 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.getStorageNamespace(), err) + return fmt.Errorf("Failed to get revision %d of release %s in namespace %s: %w", d.revision, d.release, d.storage(), err) } if d.isAllowUnreleased() { newInstall = true @@ -327,7 +319,7 @@ func (d *diffCmd) runHelm3() error { } } if err != nil { - return fmt.Errorf("Failed to get release %s in namespace %s: %w", d.release, d.getStorageNamespace(), err) + return fmt.Errorf("Failed to get release %s in namespace %s: %w", d.release, d.storage(), err) } installManifest, err := d.template(!newInstall) @@ -339,7 +331,7 @@ func (d *diffCmd) runHelm3() error { if d.threeWayMerge || d.takeOwnership { actionConfig = new(action.Configuration) localEnv := prepareEnvSettings(d.kubeContext) - storageNs := d.getStorageNamespace() + storageNs := d.storage() if storageNs == "" { storageNs = localEnv.Namespace() } @@ -361,7 +353,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.revision, d.getStorageNamespace(), d.kubeContext) + hooks, err := getHooks(d.release, d.revision, d.storage(), d.kubeContext) if err != nil { return err } diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index e45e8b6e..900cc375 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -235,53 +235,6 @@ func TestValidateRevision(t *testing.T) { } } -func TestGetStorageNamespace(t *testing.T) { - cases := []struct { - name string - namespace string - storageNamespace string - expected string - }{ - { - name: "storage namespace defaults to target namespace when unset", - namespace: "target-ns", - storageNamespace: "", - expected: "target-ns", - }, - { - name: "storage namespace overrides target namespace when set", - namespace: "target-ns", - storageNamespace: "flux-system", - expected: "flux-system", - }, - { - name: "both empty returns empty", - namespace: "", - storageNamespace: "", - expected: "", - }, - { - name: "storage namespace set with empty target namespace", - namespace: "", - storageNamespace: "flux-system", - expected: "flux-system", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - d := diffCmd{ - namespace: tc.namespace, - storageNamespace: tc.storageNamespace, - } - actual := d.getStorageNamespace() - if actual != tc.expected { - t.Errorf("expected %q, got %q", tc.expected, actual) - } - }) - } -} - func TestUpgradeCommand_StorageNamespaceFlag(t *testing.T) { cmd := newChartCommand() f := cmd.Flags() diff --git a/scripts/gen-readme.sh b/scripts/gen-readme.sh new file mode 100755 index 00000000..39ba8a03 --- /dev/null +++ b/scripts/gen-readme.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# Regenerates the cobra flag tables in README.md from the --help output of the +# diff binary, so the documented flags cannot drift from the actual flags. +# +# Usage: scripts/gen-readme.sh [path-to-diff-binary] (default: bin/diff) +# +# The README contains one flag table per command, in the order listed in +# COMMANDS below. Only the contiguous block of indented flag rows that follows +# each "Flags:" line is rewritten; any surrounding prose is left untouched. +set -euo pipefail + +BIN="${1:-bin/diff}" + +if [ ! -x "${BIN}" ]; then + echo "diff binary not found or not executable at ${BIN}. Run 'make build' first." >&2 + exit 1 +fi + +# The README documents the flags of these commands, in this order. +# The first table belongs to the (deprecated) root command, which carries the +# same flag set as "upgrade". +COMMANDS=("" local upgrade release revision rollback) + +WORKDIR="$(mktemp -d)" +trap 'rm -rf "${WORKDIR}"' EXIT + +# extract_flags prints the indented flag rows of the "Flags:" section of the +# given subcommand's --help output. An empty subcommand selects the root +# command. HELM_NAMESPACE and HELM_DIFF_STORAGE_NAMESPACE are unset so that +# no environment default leaks into the rendered flag defaults. +extract_flags() { + local subcmd="$1" + local args=() + if [ -n "${subcmd}" ]; then + args+=("${subcmd}") + fi + env -u HELM_NAMESPACE -u HELM_DIFF_STORAGE_NAMESPACE "${BIN}" ${args[@]+"${args[@]}"} --help | awk ' + /^Flags:$/ { in_flags = 1; next } + in_flags && /^ / { print; next } + in_flags { exit } + ' +} + +i=0 +for subcmd in "${COMMANDS[@]}"; do + if ! extract_flags "${subcmd}" > "${WORKDIR}/flags_${i}.txt" || [ ! -s "${WORKDIR}/flags_${i}.txt" ]; then + echo "failed to extract flag table for command '${subcmd:-}'" >&2 + exit 1 + fi + i=$((i + 1)) +done + +README="${README:-README.md}" + +awk -v workdir="${WORKDIR}" -v n="${#COMMANDS[@]}" ' + function load_rows(i, line) { + rows[i] = "" + while ((getline line < (workdir "/flags_" i ".txt")) > 0) + rows[i] = rows[i] line "\n" + close(workdir "/flags_" i ".txt") + } + BEGIN { + for (i = 0; i < n; i++) load_rows(i) + } + /^Flags:$/ && idx < n { + print + idx++ + printf "%s", rows[idx - 1] + in_rows = 1 + next + } + in_rows && /^ / { next } + { in_rows = 0; print } +' "${README}" > "${README}.new" + +mv "${README}.new" "${README}" +echo "Regenerated ${#COMMANDS[@]} flag tables in ${README}" From 5a0600d8a9fea1a2ef5d86d48668843dee9d0814 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 30 Aug 2026 08:00:56 +0800 Subject: [PATCH 5/7] fix: fail gen-readme.sh on flag table count mismatch and document make readme - gen-readme.sh now exits with an error when the number of "Flags:" tables found in README.md does not match the number of documented commands, so a lost or accidentally added table can no longer slip through the CI verification silently (both fewer and extra tables are caught). The spliced README is written to the script workdir so a failed run leaves no stray .new file behind - Document `make readme` in README.md so contributors adding or changing command flags know the flag tables must be regenerated (CI enforces this via make verify-readme) Signed-off-by: yxxhero --- README.md | 9 +++++++++ scripts/gen-readme.sh | 15 ++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 03310534..722942f9 100644 --- a/README.md +++ b/README.md @@ -541,6 +541,15 @@ To run all tests: go test -v ./... ``` +### Updating the flag tables in this README + +The per-command `Flags:` tables above are generated from the actual `--help` +output. After adding or changing a command flag, regenerate them with: +``` +make readme +``` +CI fails if the committed tables do not match the binary (`make verify-readme`). + ## Release Bump `version` in `plugin.yaml`: diff --git a/scripts/gen-readme.sh b/scripts/gen-readme.sh index 39ba8a03..3b09aeca 100755 --- a/scripts/gen-readme.sh +++ b/scripts/gen-readme.sh @@ -7,6 +7,8 @@ # The README contains one flag table per command, in the order listed in # COMMANDS below. Only the contiguous block of indented flag rows that follows # each "Flags:" line is rewritten; any surrounding prose is left untouched. +# The script fails if the number of "Flags:" tables found does not match the +# number of commands, so a lost or accidentally added table cannot slip through. set -euo pipefail BIN="${1:-bin/diff}" @@ -52,7 +54,7 @@ done README="${README:-README.md}" -awk -v workdir="${WORKDIR}" -v n="${#COMMANDS[@]}" ' +awk -v workdir="${WORKDIR}" -v n="${#COMMANDS[@]}" -v readme="${README}" ' function load_rows(i, line) { rows[i] = "" while ((getline line < (workdir "/flags_" i ".txt")) > 0) @@ -62,6 +64,7 @@ awk -v workdir="${WORKDIR}" -v n="${#COMMANDS[@]}" ' BEGIN { for (i = 0; i < n; i++) load_rows(i) } + /^Flags:$/ { total++ } /^Flags:$/ && idx < n { print idx++ @@ -71,7 +74,13 @@ awk -v workdir="${WORKDIR}" -v n="${#COMMANDS[@]}" ' } in_rows && /^ / { next } { in_rows = 0; print } -' "${README}" > "${README}.new" + END { + if (total != n || idx != n) { + printf "expected %d flag tables in %s, found %d\n", n, readme, total > "/dev/stderr" + exit 1 + } + } +' "${README}" > "${WORKDIR}/README.new" -mv "${README}.new" "${README}" +mv "${WORKDIR}/README.new" "${README}" echo "Regenerated ${#COMMANDS[@]} flag tables in ${README}" From 943b384c5f390ad2ef4ab5ddc0926d0cf14c53d9 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 30 Aug 2026 08:08:31 +0800 Subject: [PATCH 6/7] ci: add integration test for the storage namespace separation Add a Flux-style integration scenario to the kind-based integration tests: install a release into the target namespace (prod-apps), move its helm release storage secrets (sh.helm.release.v1.*) into a separate storage namespace (flux-system), and verify: - helm get / helm diff upgrade fail against the target namespace alone - helm diff upgrade succeeds with --storage-namespace - the HELM_DIFF_STORAGE_NAMESPACE env var is honored end-to-end - the three-way-merge pipeline works with separated storage against a real cluster (live object reads in the target namespace, release manifests fetched from the storage namespace) Signed-off-by: yxxhero --- .github/workflows/ci.yaml | 43 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f63e4934..2562ce2a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -153,3 +153,46 @@ jobs: - name: helm diff upgrade -C 3 --set replicaCount=2 --install helm-diff ./helm-diff run: helm diff upgrade -C 3 --set replicaCount=2 --install helm-diff ./helm-diff + + # Flux-style setup: the release storage (sh.helm.release.v1.* secrets) lives + # in a separate storage namespace (flux-system) while the workloads are + # deployed into the target namespace (prod-apps). Exercises --storage-namespace, + # HELM_DIFF_STORAGE_NAMESPACE and the three-way-merge pipeline against a + # real cluster. + - name: Install release into prod-apps and move its storage to flux-system + run: | + helm upgrade -i storage-diff ./helm-diff -n prod-apps --create-namespace + kubectl create namespace flux-system + for secret in $(kubectl get secrets -n prod-apps -o name | grep '^secret/sh\.helm\.release\.v1\.storage-diff\.'); do + kubectl get -n prod-apps "${secret}" -o json \ + | jq --arg ns flux-system '.metadata.namespace = $ns | del(.metadata.resourceVersion, .metadata.uid, .metadata.creationTimestamp)' \ + | kubectl apply -f - + kubectl delete -n prod-apps "${secret}" + done + + - name: Verify release storage is only visible in flux-system + run: | + if helm get manifest storage-diff -n prod-apps >/dev/null 2>&1; then + echo "unexpected: release storage still found in prod-apps" >&2 + exit 1 + fi + helm get manifest storage-diff -n flux-system >/dev/null + + - name: helm diff upgrade fails without --storage-namespace + run: | + if helm diff upgrade storage-diff ./helm-diff -n prod-apps >/dev/null 2>&1; then + echo "unexpected: diff succeeded although the release storage is not in prod-apps" >&2 + exit 1 + fi + + - name: helm diff upgrade --storage-namespace + run: helm diff upgrade storage-diff ./helm-diff -n prod-apps --storage-namespace flux-system + + - name: helm diff upgrade with HELM_DIFF_STORAGE_NAMESPACE env var + run: HELM_DIFF_STORAGE_NAMESPACE=flux-system helm diff upgrade storage-diff ./helm-diff -n prod-apps + + - name: helm diff upgrade --storage-namespace --three-way-merge + run: | + set -o pipefail + helm diff upgrade storage-diff ./helm-diff -n prod-apps --storage-namespace flux-system --three-way-merge --set replicaCount=2 | tee /tmp/three-way.out + grep -q 'replicas: 2' /tmp/three-way.out From 3abf92651edcd1c3d16e6210d1b3e15e9519aab5 Mon Sep 17 00:00:00 2001 From: yxxhero Date: Sun, 30 Aug 2026 08:26:49 +0800 Subject: [PATCH 7/7] style: flatten printFakeHelmOutput with guard clauses Split the version-output handling into a printFakeHelmVersion helper so both functions read as flat guard clauses instead of nested if/else blocks. No behavior change. Signed-off-by: yxxhero --- cmd/main_test.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/cmd/main_test.go b/cmd/main_test.go index 95ce91d9..c49bc6b9 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -20,21 +20,26 @@ func shouldRunFakeHelm() bool { return !strings.HasPrefix(os.Args[1], "-test.") } +// printFakeHelmVersion prints helm version build info, so that the version +// checks in cmd (see getHelmVersion) work against the fake helm. +// The version output can be overridden via HELM_DIFF_FAKE_VERSION_OUTPUT. +func printFakeHelmVersion() { + if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { + fmt.Print(v) + return + } + fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) +} + // printFakeHelmOutput prints the output for a fake helm invocation. -// A `helm version` call prints helm version build info, so that the version -// checks in cmd (see getHelmVersion) work against the fake helm. The version -// output can be overridden via HELM_DIFF_FAKE_VERSION_OUTPUT. -// Any other invocation prints HELM_DIFF_FAKE_OUTPUT. +// A `helm version` call prints helm version build info; any other +// invocation prints HELM_DIFF_FAKE_OUTPUT. func printFakeHelmOutput() { if len(os.Args) > 1 && os.Args[1] == "version" { - if v := os.Getenv("HELM_DIFF_FAKE_VERSION_OUTPUT"); v != "" { - fmt.Print(v) - } else { - fmt.Println(`version.BuildInfo{Version:"v3.18.0"}`) - } - } else { - fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) + printFakeHelmVersion() + return } + fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT")) } func TestMain(m *testing.M) {