feat: add --storage-namespace flag and HELM_DIFF_STORAGE_NAMESPACE env var by asouchang · Pull Request #1056 · databus23/helm-diff

asouchang · 2026-08-27T08:44:07Z

Summary

In Kubernetes GitOps tooling (such as FluxCD's HelmRelease controller), workload resources are deployed to targetNamespace while Helm release storage records (Secrets/ConfigMaps sh.helm.release.v1.<name>.v<n>) are persisted in a separate storageNamespace (e.g. flux-system).

Previously, running helm diff upgrade -n <targetNamespace> attempted to fetch live release state from <targetNamespace>, failing with release: not found. Conversely, passing -n <storageNamespace> caused helm template to render manifests into the storage namespace instead of the target namespace.

This PR adds first-class support for --storage-namespace and the HELM_DIFF_STORAGE_NAMESPACE environment variable across upgrade, revision, and rollback commands.

Changes

Add --storage-namespace flag and HELM_DIFF_STORAGE_NAMESPACE env var across upgrade, revision, and rollback.
Fetch live release manifests, hooks, values, and 3-way merge storage from storageNamespace while keeping helm template manifest rendering and unnamespaced resource parsing in targetNamespace (-n / --namespace / HELM_NAMESPACE).
Fallback to targetNamespace when omitted for 100% backward compatibility.
Add comprehensive unit tests covering flag parsing, env var resolution, and storage namespace fallback logic.
Update documentation in README.md.

…v 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>

Copilot

🟡 Changes recommended

The new env-var behavior is not actually exercised by the added tests (tests re-implement resolution inline), creating a coverage gap for a key compatibility path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds first-class support for a separate Helm release storage namespace (Secrets/ConfigMaps like sh.helm.release.v1.*) so GitOps-style deployments can diff against releases stored outside the target workload namespace, while keeping templating/rendering in the target namespace for backward compatibility.

Changes:

Introduces --storage-namespace and HELM_DIFF_STORAGE_NAMESPACE across upgrade, revision, and rollback, with fallback to the target namespace (-n/--namespace).
Routes release reads (manifests/hooks/values and 3-way merge storage) through the storage namespace while leaving helm template behavior tied to the target namespace.
Adds new/updated unit tests and updates README flag documentation and examples.

File summaries

File	Description
README.md	Documents `--storage-namespace` / `HELM_DIFF_STORAGE_NAMESPACE` and provides an example.
cmd/upgrade.go	Adds storage namespace plumbing for upgrade, including release/hook fetches and 3-way-merge action config initialization.
cmd/helm.go	Updates existing-values retrieval to use the storage namespace rather than the target namespace.
cmd/upgrade_test.go	Adds unit tests for storage namespace helper and flag/env basics (but env wiring isn’t exercised end-to-end).
cmd/rollback.go	Adds `--namespace` + `--storage-namespace` and uses storage namespace for fetching releases while parsing manifests in the target namespace.
cmd/rollback_test.go	Adds rollback flag tests and a storage namespace test that currently re-implements env wiring inline.
cmd/revision.go	Adds `--namespace` + `--storage-namespace` and uses storage namespace for fetching releases while parsing manifests in the target namespace.
cmd/revision_test.go	Adds revision flag tests and a storage namespace test that currently re-implements env wiring inline.

Review details

Files reviewed: 8/8 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…lm 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>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

yxxhero

Thanks for the PR — this fills a real gap for FluxCD-style setups where the release storage (sh.helm.release.v1.* secrets) lives in a separate namespace from the workload target namespace.

I audited all namespace consumers in the three touched commands, and the separation looks correct:

Storage namespace (correctly switched): helm get manifest/hooks/values (getRelease, getHooks, writeExistingValues) and actionConfig.Init for the three-way-merge / take-ownership path.
Target namespace (correctly kept): helm template / helm upgrade --dry-run rendering, manifest.Parse namespace defaulting, and checkOwnership live-object lookups.

Backward compatibility is preserved via the resolveStorageNamespace fallback, and the error messages now report the namespace that was actually queried. The second commit also addresses the earlier review feedback (fake-helm capture_args e2e tests + shared helpers).

Verified locally on the PR branch: go vet ./..., go test ./... and go test -race ./cmd/... all pass.

The remaining comments are non-blocking suggestions. One scope note for a potential follow-up: helm diff release still resolves both releases through HELM_NAMESPACE / the namespace/release argument syntax only, so it has the same storage/target limitation this PR fixes for upgrade/revision/rollback — fine to leave out of this PR, but worth a follow-up issue for consistency.

yxxhero · 2026-08-28T23:36:48Z

+
+// 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) {


Minor: the namespace half of this helper is effectively a no-op in production — all three commands register --namespace with os.Getenv("HELM_NAMESPACE") as the flag default (evaluated at command construction), so *namespace already holds the env value by the time RunE runs, and re-reading the same env var can't change it.

It's harmless (and the tests rely on the re-read after t.Setenv), but having two mechanisms resolve the same thing invites drift. You could simplify by giving --storage-namespace the same treatment (os.Getenv("HELM_DIFF_STORAGE_NAMESPACE") as flag default) and dropping this helper entirely — an explicit --storage-namespace="" would still override the env since it sets the value to empty, which then falls back to the target namespace.

yxxhero · 2026-08-28T23:36:48Z

+		if storageNs == "" {
+			storageNs = localEnv.Namespace()
+		}
+		if err := actionConfig.Init(localEnv.RESTClientGetter(), storageNs, os.Getenv("HELM_DRIVER")); err != nil {


This looks right — actionConfig.Init's namespace is where the release driver looks up the release record, so the storage namespace belongs here, and the localEnv.Namespace() fallback preserves the old behavior when neither namespace is set. manifest.Generate fetches live objects using each resource's own namespace from the manifest, so target-namespace workloads are unaffected. 👍

yxxhero · 2026-08-28T23:36:48Z

-			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" {


Nit: this version-output block is now duplicated between the capture_args and default branches — consider extracting a small helper so future modes share it. Also, HELM_DIFF_FAKE_VERSION_OUTPUT is never exercised by any test; fine as an escape hatch, but a one-line test (or dropping the knob) would avoid carrying untested surface.

yxxhero · 2026-08-28T23:36:48Z

      --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)


Nit: the newly added -n, --namespace flag on upgrade/revision/rollback isn't listed in these flag tables (only the --storage-namespace rows were inserted). Consider regenerating the help blocks so the tables match the actual --help output.

yxxhero · 2026-08-28T23:36:48Z

 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.


Might be worth a caveat here: HELM_DIFF_USE_UPGRADE_DRY_RUN=true doesn't honor the storage/target separation — rendering then goes through helm upgrade --dry-run --namespace <target>, and helm resolves the release storage in the target namespace. With the release only present in the storage namespace, that invocation fails with "has no deployed releases" unless --install/--allow-unreleased is set (in which case it renders as a fresh install, which is still usable for diffing, just slightly different semantics for .Release.Revision etc.). A one-line note would help Flux users hitting this combination.

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 <aiopsclub@163.com>

…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 <aiopsclub@163.com>

…e 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 <aiopsclub@163.com>

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 <aiopsclub@163.com>

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 <aiopsclub@163.com>

yxxhero requested a lite review from Copilot August 27, 2026 23:02

Copilot started reviewing on behalf of yxxhero August 27, 2026 23:02 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread cmd/upgrade_test.go Outdated

Comment thread cmd/revision_test.go Outdated

Comment thread cmd/rollback_test.go Outdated

Comment thread cmd/revision.go Outdated

yxxhero requested a balanced review from Copilot August 28, 2026 13:00

Copilot AI reviewed Aug 28, 2026

yxxhero reviewed Aug 28, 2026

View reviewed changes

yxxhero added 5 commits August 30, 2026 07:41

style: flatten printFakeHelmOutput with guard clauses

3abf926

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 <aiopsclub@163.com>

yxxhero merged commit d5935f1 into databus23:master Aug 30, 2026
25 checks passed

yxxhero mentioned this pull request Aug 30, 2026

chore: bump plugin version to 3.15.12 #1057

Merged

Navigation Menu

feat: add --storage-namespace flag and HELM_DIFF_STORAGE_NAMESPACE env var - #1056

feat: add --storage-namespace flag and HELM_DIFF_STORAGE_NAMESPACE env var#1056
yxxhero merged 7 commits into
databus23:masterfrom
asouchang:feat/storage-namespace

asouchang commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

yxxhero left a comment

Uh oh!

yxxhero Aug 28, 2026

Uh oh!

yxxhero Aug 28, 2026

Uh oh!

yxxhero Aug 28, 2026

Uh oh!

yxxhero Aug 28, 2026

Uh oh!

yxxhero Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

asouchang commented Aug 27, 2026

Summary

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

yxxhero left a comment

Choose a reason for hiding this comment

Uh oh!

yxxhero Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yxxhero Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yxxhero Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yxxhero Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

yxxhero Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants