feat: add local subcommand to compare two chart folders by yxxhero · Pull Request #982 · databus23/helm-diff

yxxhero · 2026-04-18T03:02:28Z

Summary

Add a new local subcommand that diffs two local chart directories by rendering both charts using helm template and showing the differences between the resulting manifests.
Useful for comparing different versions of a chart, previewing changes before committing, and validating chart modifications.
Based on feat: add local subcommand to compare two chart folders #866 with additional fixes for linter compliance and test coverage.

Usage

helm diff local ./chart-v1 ./chart-v2
helm diff local ./chart-v1 ./chart-v2 -f values.yaml
helm diff local /path/to/chart-a /path/to/chart-b --set replicas=3

- Lowercase error messages to follow Go conventions - Extract stdin reading into prepareStdinValues() to avoid double-read when rendering both charts - Add HELM_BIN env var validation with "helm" default fallback Signed-off-by: yxxhero <aiopsclub@163.com>

…string Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Adds a new helm diff local subcommand to diff the rendered manifests from two local chart directories using helm template.

Changes:

Register a new local Cobra subcommand in the root command.
Implement local chart rendering + manifest diffing logic (helm template both charts, then diff).
Add tests and README documentation for the new command.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`cmd/root.go`	Wires the new `local` subcommand into the CLI.
`cmd/local.go`	Implements `helm diff local` flag parsing, Helm templating, and diff execution.
`cmd/local_test.go`	Adds unit tests for argument validation and command behavior.
`README.md`	Documents the new command and includes help/usage text.

Copilot · 2026-04-18T03:11:49Z

+			tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")
+			if err != nil {
+				return err
+			}
+			defer func() { _ = os.Remove(tmpfile.Name()) }()
+
+			if _, err := tmpfile.Write(data); err != nil {


prepareStdinValues creates a temp file for -f - and then defers os.Remove(tmpfile.Name()). Because the function returns before renderChart is called, the deferred removal runs immediately and deletes the values file before Helm can read it, causing helm template --values <tmp> to fail whenever stdin values are used. Keep the temp file alive until after both charts are rendered (e.g., move stdin handling into renderChart, or have prepareStdinValues return a cleanup func that run() defers).

Copilot · 2026-04-18T03:11:49Z

+	err := os.WriteFile(fakeHelm, []byte(`#!/bin/sh
+cat <<EOF
+`+manifestYAML+`
+EOF
+`), 0755)


These tests create a #!/bin/sh fake Helm script and rely on Unix permissions/paths (e.g., tmpDir + "/helm", mode 0755). This is not portable and will fail on Windows CI (no /bin/sh, different executable semantics). Consider switching to the cross-platform fake-helm approach used in main_test.go (re-exec the test binary when an env var is set and stub the template subcommand), or otherwise generate a platform-appropriate executable per runtime.GOOS.

Copilot · 2026-04-18T03:11:50Z

+      --kube-version string                      Kubernetes version used for Capabilities.KubeVersion
+      --namespace string                         namespace to use for template rendering
+      --normalize-manifests                      normalize manifests before running diff to exclude style differences from the output
+      --output string                            Possible values: diff, simple, template, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff")


The --output flag description in the new local help block lists "diff, simple, template, dyff" but AddDiffOptions (cmd/options.go:16) advertises additional formats (e.g., json, structured). Update this README snippet to match the actual help output / supported formats so users don't miss available options.

Suggested change

--output string Possible values: diff, simple, template, dyff. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff")

--output string Possible values: diff, simple, template, dyff, json, structured. When set to "template", use the env var HELM_DIFF_TPL to specify the template. (default "diff")

- Fix prepareStdinValues temp file cleanup: return cleanup func so the temp file survives until after both charts are rendered - Rewrite tests to use cross-platform TestMain re-exec pattern instead of #!/bin/sh shell scripts - Update README --output flag description to include json, structured

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Copilot · 2026-04-18T06:00:50Z

+	oldStdout := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	cmd := localCmd()
+	cmd.SetArgs([]string{chart1, chart2})
+
+	err := cmd.Execute()
+	w.Close()
+	os.Stdout = oldStdout
+
+	if err != nil {
+		t.Fatalf("Expected no error but got: %v", err)
+	}
+
+	var buf bytes.Buffer
+	buf.ReadFrom(r)
+	if buf.String() != "" {
+		t.Errorf("Expected no output when charts are identical, got: %q", buf.String())
+	}


This test ignores errors from os.Pipe() and buf.ReadFrom(r). If either fails, the assertions may be misleading. Please check these errors (and close r) to make test failures clearer and avoid leaking file descriptors.

Copilot · 2026-04-18T06:00:51Z

+		t.Fatalf("Expected no error but got: %v", err)
+	}
+
+	args1, _ := os.ReadFile(argsFile)


The test ignores the error from os.ReadFile(argsFile). If the fake helm didn't write the file (or write failed), this will silently treat the args as empty and could produce a confusing failure. Please assert ReadFile succeeds before checking the contents.

Suggested change

args1, _ := os.ReadFile(argsFile)

args1, err := os.ReadFile(argsFile)

if err != nil {

t.Fatalf("Expected fake helm args file to be readable, but got: %v", err)

}

Copilot · 2026-04-18T06:00:51Z

+			if v, _ := cmd.Flags().GetBool("version"); v {
+				fmt.Println(Version)
+				return nil
+			}
+


RunE checks cmd.Flags().GetBool("version"), but this command doesn't define a boolean --version flag (and the project already has a version subcommand; upgrade also uses --version as a string flag for chart version). As written, this code path can never be true and also silently ignores the flag lookup error; please remove this check or replace it with the intended mechanism (e.g., rely on the version subcommand).

Suggested change

if v, _ := cmd.Flags().GetBool("version"); v {

fmt.Println(Version)

return nil

}

Copilot · 2026-04-18T06:00:51Z

+	for i, valueFile := range l.valueFiles {
+		if strings.TrimSpace(valueFile) == "-" {
+			data, err := io.ReadAll(os.Stdin)
+			if err != nil {
+				return nil, err
+			}
+
+			tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")
+			if err != nil {
+				return nil, err
+			}
+
+			if _, err := tmpfile.Write(data); err != nil {
+				_ = tmpfile.Close()
+				return nil, err
+			}
+
+			if err := tmpfile.Close(); err != nil {
+				return nil, err
+			}
+
+			l.valueFiles[i] = tmpfile.Name()
+			name := tmpfile.Name()
+			return func() { _ = os.Remove(name) }, nil
+		}
+	}


prepareStdinValues returns immediately after handling the first "-" values file, so any subsequent "-" entries in l.valueFiles remain unchanged and will later be passed through to helm template as a literal filename "-". Consider reading stdin once, writing one temp file, and replacing all "-" entries (or otherwise ensure every "-" is handled) before returning.

Suggested change

for i, valueFile := range l.valueFiles {

if strings.TrimSpace(valueFile) == "-" {

data, err := io.ReadAll(os.Stdin)

if err != nil {

return nil, err

}

tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")

if err != nil {

return nil, err

}

if _, err := tmpfile.Write(data); err != nil {

_ = tmpfile.Close()

return nil, err

}

if err := tmpfile.Close(); err != nil {

return nil, err

}

l.valueFiles[i] = tmpfile.Name()

name := tmpfile.Name()

return func() { _ = os.Remove(name) }, nil

}

}

var name string

for i, valueFile := range l.valueFiles {

if strings.TrimSpace(valueFile) == "-" {

if name == "" {

data, err := io.ReadAll(os.Stdin)

if err != nil {

return nil, err

}

tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")

if err != nil {

return nil, err

}

if _, err := tmpfile.Write(data); err != nil {

_ = tmpfile.Close()

return nil, err

}

if err := tmpfile.Close(); err != nil {

return nil, err

}

name = tmpfile.Name()

}

l.valueFiles[i] = name

}

}

if name != "" {

return func() { _ = os.Remove(name) }, nil

}

Copilot · 2026-04-18T06:00:51Z

+			countFile := os.Getenv("HELM_DIFF_FAKE_COUNT_FILE")
+			data, _ := os.ReadFile(countFile)
+			count := 0
+			if len(data) > 0 {
+				fmt.Sscanf(string(data), "%d", &count)
+			}
+			count++
+			os.WriteFile(countFile, []byte(fmt.Sprintf("%d", count)), 0644)
+			if count == 1 {
+				fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT_1"))
+			} else {
+				fmt.Print(os.Getenv("HELM_DIFF_FAKE_OUTPUT_2"))
+			}


In the fake-helm "dual" mode, errors from os.ReadFile, fmt.Sscanf, and os.WriteFile are ignored. If the count file can't be read/written, the helper can silently return the wrong output and make tests flaky/hard to debug. Please handle these errors (print to stderr and exit non-zero) so failures are explicit.

Copilot · 2026-04-18T06:00:52Z

+		case "capture_args":
+			argsFile := os.Getenv("HELM_DIFF_FAKE_ARGS_FILE")
+			if argsFile != "" {
+				os.WriteFile(argsFile, []byte(strings.Join(os.Args[1:], " ")), 0644)


In the fake-helm "capture_args" mode, the return value from os.WriteFile is ignored. If writing the args file fails, the test may pass/fail for the wrong reason. Please check the error and exit non-zero (or at least print a diagnostic to stderr) when it occurs.

Suggested change

os.WriteFile(argsFile, []byte(strings.Join(os.Args[1:], " ")), 0644)

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)

}

- Remove unreachable --version flag check in local command - Fix prepareStdinValues to handle all stdin value files - Add proper error handling for os.Pipe and ReadFrom in tests - Handle errors in fake helm test helpers Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Copilot · 2026-04-18T06:15:06Z

+					_ = tmpfile.Close()
+					return nil, err
+				}
+
+				if err := tmpfile.Close(); err != nil {


In prepareStdinValues, if creating the temp values file succeeds but writing to it or closing it fails, the function returns without removing the temp file. This can leak temporary files on error paths. Ensure the temp file is removed in all failure cases (e.g., call os.Remove(tmpfile.Name()) before returning on write/close errors).

Suggested change

_ = tmpfile.Close()

return nil, err

}

if err := tmpfile.Close(); err != nil {

_ = tmpfile.Close()

_ = os.Remove(tmpfile.Name())

return nil, err

}

if err := tmpfile.Close(); err != nil {

_ = os.Remove(tmpfile.Name())

Copilot · 2026-04-18T06:15:07Z

+	}
+
+	specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)
+	specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)


After parsing manifest1/manifest2 into specs maps, consider nil-ing the raw manifest byte slices (as done in other commands) so large rendered manifests can be garbage-collected before diff computation. This reduces peak memory usage when diffing large charts.

Suggested change

specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

manifest1 = nil

manifest2 = nil

Copilot · 2026-04-18T06:15:07Z

+func (l *local) prepareStdinValues() (func(), error) {
+	var name string
+
+	for i, valueFile := range l.valueFiles {
+		if strings.TrimSpace(valueFile) == "-" {
+			if name == "" {
+				data, err := io.ReadAll(os.Stdin)
+				if err != nil {
+					return nil, err
+				}
+
+				tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")
+				if err != nil {
+					return nil, err
+				}
+
+				if _, err := tmpfile.Write(data); err != nil {
+					_ = tmpfile.Close()
+					return nil, err
+				}
+
+				if err := tmpfile.Close(); err != nil {
+					return nil, err
+				}
+
+				name = tmpfile.Name()
+			}
+
+			l.valueFiles[i] = name
+		}
+	}
+
+	if name != "" {
+		return func() { _ = os.Remove(name) }, nil
+	}
+	return nil, nil
+}


prepareStdinValues adds non-trivial behavior for handling --values - (stdin) so that both charts can be rendered without re-reading stdin. There are currently no tests covering this path; add a test that sets --values - with a controlled stdin and asserts localCmd completes and passes a concrete temp file path (not '-') to both helm template invocations.

Copilot · 2026-04-18T06:15:07Z

+	oldStdout := os.Stdout
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("Failed to create pipe: %v", err)
+	}
+	os.Stdout = w
+
+	cmd := localCmd()
+	cmd.SetArgs([]string{chart1, chart2})
+
+	err = cmd.Execute()
+	w.Close()
+	os.Stdout = oldStdout


These tests manually replace os.Stdout using os.Pipe. There is already a captureStdout helper in cmd/helpers_test.go that restores stdout via defer, which avoids leaving global stdout redirected if the test fails early. Consider using that helper here to reduce duplication and make the tests more robust.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-18T06:23:29Z

+				tmpfile, err := os.CreateTemp("", "helm-diff-stdin-values")
+				if err != nil {
+					return nil, err
+				}
+
+				if _, err := tmpfile.Write(data); err != nil {
+					_ = tmpfile.Close()
+					return nil, err
+				}
+
+				if err := tmpfile.Close(); err != nil {
+					return nil, err
+				}


In prepareStdinValues, if creating the temp file succeeds but tmpfile.Write or tmpfile.Close returns an error, the function returns without removing the temp file. This can leave stray files in the system temp dir. Consider ensuring the temp file is removed on all error paths (e.g., defer os.Remove(tmpfile.Name()) immediately after CreateTemp and cancel it on success).

Copilot · 2026-04-18T06:23:29Z

+		return fmt.Errorf("failed to render chart %s: %w", l.chart1, err)
+	}
+
+	manifest2, err := l.renderChart(l.chart2)
+	if err != nil {
+		return fmt.Errorf("failed to render chart %s: %w", l.chart2, err)


The chart path in the render error is interpolated with %s, which can be ambiguous when paths contain spaces or special characters. Using %q would make the error message unambiguous (and consistent with other errors that quote values).

Suggested change

return fmt.Errorf("failed to render chart %s: %w", l.chart1, err)

}

manifest2, err := l.renderChart(l.chart2)

if err != nil {

return fmt.Errorf("failed to render chart %s: %w", l.chart2, err)

return fmt.Errorf("failed to render chart %q: %w", l.chart1, err)

}

manifest2, err := l.renderChart(l.chart2)

if err != nil {

return fmt.Errorf("failed to render chart %q: %w", l.chart2, err)

- Fix temp file leak in prepareStdinValues on error paths - Use %q for quoted chart paths in error messages - Nil out manifest byte slices after parsing to reduce peak memory - Refactor tests to use captureStdout helper - Add tests for prepareStdinValues stdin handling Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-18T07:33:39Z

+	var specs1, specs2 map[string]*manifest.MappingResult
+	func() {
+		specs1 = manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)
+		specs2 = manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)
+		manifest1 = nil
+		manifest2 = nil
+	}()


The manifest1 = nil / manifest2 = nil assignments are likely to trip ineffassign (enabled in .golangci.yaml) and the anonymous wrapper func doesn’t avoid that. Either remove the nil-ing entirely, or keep it but add the same //nolint:ineffassign annotation used elsewhere (e.g. cmd/upgrade.go) and drop the extra closure.

Suggested change

var specs1, specs2 map[string]*manifest.MappingResult

func() {

specs1 = manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)

specs2 = manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

manifest1 = nil

manifest2 = nil

}()

specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)

specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

…r commands Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Copilot · 2026-04-18T13:06:31Z

+	manifest1, err := l.renderChart(l.chart1)
+	if err != nil {
+		return fmt.Errorf("failed to render chart %q: %w", l.chart1, err)
+	}
+
+	manifest2, err := l.renderChart(l.chart2)
+	if err != nil {
+		return fmt.Errorf("failed to render chart %q: %w", l.chart2, err)
+	}
+
+	excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook}
+	if l.includeTests {
+		excludes = []string{}
+	}
+
+	specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)
+	specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)
+	manifest1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation


run() renders both charts before parsing either manifest, which keeps both raw manifest byte slices alive at once. For large charts this can double peak memory, and the later manifest1=nil/manifest2=nil GC hint only helps after both parses. Consider rendering+parsing chart1 first (then drop manifest1) before rendering chart2, or otherwise streaming/early-parsing to reduce peak memory.

Suggested change

manifest1, err := l.renderChart(l.chart1)

if err != nil {

return fmt.Errorf("failed to render chart %q: %w", l.chart1, err)

}

manifest2, err := l.renderChart(l.chart2)

if err != nil {

return fmt.Errorf("failed to render chart %q: %w", l.chart2, err)

}

excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook}

if l.includeTests {

excludes = []string{}

}

specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)

specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

manifest1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before diff computation

excludes := []string{manifest.Helm3TestHook, manifest.Helm2TestSuccessHook}

if l.includeTests {

excludes = []string{}

}

manifest1, err := l.renderChart(l.chart1)

if err != nil {

return fmt.Errorf("failed to render chart %q: %w", l.chart1, err)

}

specs1 := manifest.Parse(manifest1, l.namespace, l.normalizeManifests, excludes...)

manifest1 = nil //nolint:ineffassign // nil to allow GC to reclaim raw bytes before rendering/parsing the second chart

manifest2, err := l.renderChart(l.chart2)

if err != nil {

return fmt.Errorf("failed to render chart %q: %w", l.chart2, err)

}

specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)

Copilot · 2026-04-18T13:06:31Z

+func (l *local) renderChart(chartPath string) ([]byte, error) {
+	flags := []string{}
+
+	if l.includeCRDs {
+		flags = append(flags, "--include-crds")
+	}
+
+	if l.namespace != "" {
+		flags = append(flags, "--namespace", l.namespace)


renderChart largely duplicates the helm template flag/arg construction logic that already exists in (*diffCmd).template (cmd/helm.go:186+), but without its Helm-version compatibility behavior (e.g., dry-run/validation handling) and other flags. This duplication risks the two code paths drifting and producing different rendered manifests for the same inputs. Consider extracting a shared helper for building/executing helm template (reused by both upgrade and local) so rendering semantics stay consistent.

Copilot · 2026-04-18T13:06:31Z

+func TestMain(m *testing.M) {
+	if os.Getenv("HELM_DIFF_FAKE_HELM") == "1" {


TestMain short-circuits the entire cmd package test run whenever HELM_DIFF_FAKE_HELM=1 is present in the environment, which can silently skip tests if that variable is set externally. To avoid accidental bypass, consider using a more unique helper-process env var name and/or add a guard that only enters fake-helm mode when invoked with an expected helm subcommand (e.g., os.Args[1] == "template").

Suggested change

func TestMain(m *testing.M) {

if os.Getenv("HELM_DIFF_FAKE_HELM") == "1" {

func shouldRunFakeHelm() bool {

if os.Getenv("HELM_DIFF_FAKE_HELM") != "1" {

return false

}

if len(os.Args) < 2 {

return false

}

return !strings.HasPrefix(os.Args[1], "-test.")

}

func TestMain(m *testing.M) {

if shouldRunFakeHelm() {

…elm mode Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

bo0tzz and others added 7 commits March 30, 2026 07:50

feat: add local subcommand to compare two chart folders

3e262c0

fix: format

9b7e101

fix: pass []byte directly to manifest.Parse instead of converting to …

cc56dae

…string Signed-off-by: yxxhero <aiopsclub@163.com>

fix: check error return values for errcheck linter

d4f7453

Signed-off-by: yxxhero <aiopsclub@163.com>

test: add integration tests for local subcommand

8edb615

Signed-off-by: yxxhero <aiopsclub@163.com>

fix: use errors.As instead of type assertion for errorlint

21e7fd1

Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero requested a review from Copilot April 18, 2026 03:06

Copilot started reviewing on behalf of yxxhero April 18, 2026 03:07 View session