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

bo0tzz · 2025-10-03T18:50:33Z

This adds a new local subcommand that diffs two local chart directories. My intended usecase is mainly for development and CI/CD, to see what effects a change has.

This code was written entirely by an LLM; if that is a dealbreaker, feel free to just close this PR outright.

If there is appetite for it, I'd like to next build on top of this with a git subcommand that diffs two git references against eachother (by checking them out to a temporary folder and then running local against them).

yxxhero · 2025-10-10T08:12:11Z

@bo0tzz maybe we should add an e2e test?

yxxhero · 2025-10-10T08:12:37Z

The rest is LGTM. @bo0tzz

Copilot

Pull Request Overview

This PR adds a new local subcommand to the helm-diff plugin that allows comparing two local chart directories. The feature enables developers to see differences between chart versions during development and CI/CD workflows by rendering both charts with helm template and showing the resulting manifest differences.

Key changes:

Implements a new local subcommand with comprehensive flag support for chart rendering options
Adds comprehensive test coverage for argument validation and command execution
Updates documentation to include the new subcommand usage and examples

Reviewed Changes

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

File	Description
cmd/root.go	Registers the new localCmd() subcommand in the main command structure
cmd/local.go	Complete implementation of the local subcommand with chart rendering and diffing logic
cmd/local_test.go	Test suite covering argument validation and basic execution scenarios
README.md	Documentation update adding the local subcommand help text and usage examples

Copilot · 2025-10-10T08:13:24Z

cmd/local.go

+func (l *local) run() error {
+	manifest1, err := l.renderChart(l.chart1)
+	if err != nil {
+		return fmt.Errorf("Failed to render chart %s: %w", l.chart1, err)


Error messages should start with lowercase letters to follow Go conventions.

Suggested change

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

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

Copilot · 2025-10-10T08:13:24Z

cmd/local.go

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


Error messages should start with lowercase letters to follow Go conventions.

Suggested change

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

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

Copilot · 2025-10-10T08:13:25Z

cmd/local.go

+	args := []string{"template", l.release, chartPath}
+	args = append(args, flags...)
+
+	cmd := exec.Command(os.Getenv("HELM_BIN"), args...)


Using os.Getenv("HELM_BIN") without validation could allow execution of arbitrary commands. Consider validating the HELM_BIN path or providing a default fallback.

Suggested change

cmd := exec.Command(os.Getenv("HELM_BIN"), args...)

helmBin := os.Getenv("HELM_BIN")

if helmBin == "" {

helmBin = "helm"

}

cmd := exec.Command(helmBin, args...)

bo0tzz · 2025-10-10T11:40:36Z

@yxxhero I'd be down for e2e tests, but I don't think the project has any yet right? How would you like those to look?

yxxhero · 2025-10-11T03:29:47Z

@bo0tzz it will be better if we add e2e test. this is a new feature. thanks so much.

yxxhero · 2025-11-27T14:51:11Z

@copilot open a new pull request to apply changes based on the comments in this thread

yxxhero · 2025-11-27T14:58:30Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot

Pull request overview

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

Copilot · 2025-11-27T15:03:17Z

cmd/local.go

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


Error messages should start with lowercase letters following Go conventions and consistency with other error messages in the codebase.

For example, change:

"Failed to render chart %s: %w" to "failed to render chart %s: %w"

This matches the pattern used in cmd/upgrade.go:292 and cmd/helm3.go:31-41.

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

Copilot · 2025-11-27T15:03:17Z

cmd/local.go

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


Error message should start with a lowercase letter following Go conventions and consistency with other error messages in the codebase.

For example, change:

"Failed to render chart %s: %w" to "failed to render chart %s: %w"

This matches the pattern used in cmd/upgrade.go:292 and cmd/helm3.go:31-41.

Suggested change

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

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

Copilot · 2025-11-27T15:03:18Z

cmd/local.go

+	manifest1, err := l.renderChart(l.chart1)
+	if err != nil {
+		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 renderChart method is called twice (for chart1 and chart2), but it reads from stdin when -f - is used (line 169). Since stdin can only be read once, the second call to renderChart will fail or receive empty input.

This is different from the upgrade command where renderChart is only called once. For the local command, you would need to either:

Read stdin once before calling renderChart and pass the content to both calls, or

Document that stdin values (-f -) are not supported for this command, or

Reuse the same temp file for both chart renders

Consider refactoring to handle stdin reading outside of the loop or before multiple renderChart calls.

- 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

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

Copilot · 2026-03-30T01:38:48Z

README.md

+      --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 help text here omits supported formats (json, structured) that are listed for other commands and are defined in AddDiffOptions (see cmd/options.go). This makes the README inconsistent with the actual CLI/help output. Update this line to reflect the complete set of supported output formats.

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

Copilot · 2026-03-30T01:38:49Z

cmd/local.go

+			if err != nil {
+				return err
+			}
+			defer func() { _ = os.Remove(tmpfile.Name()) }()


prepareStdinValues creates a temp file for -f - but defers os.Remove inside this function. Because the defer runs when prepareStdinValues returns (before renderChart is executed), the temp file path stored in l.valueFiles[i] can be deleted before helm template tries to read it, causing helm template to fail. Keep the temp file alive until after both renderChart calls complete (e.g., defer cleanup in run() or handle stdin values within renderChart).

Suggested change

defer func() { _ = os.Remove(tmpfile.Name()) }()

Copilot · 2026-03-30T01:38:49Z

cmd/local.go

+	for i, valueFile := range l.valueFiles {
+		if strings.TrimSpace(valueFile) == "-" {
+			data, err := io.ReadAll(os.Stdin)
+			if err != nil {
+				return err
+			}
+
+			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 {
+				_ = tmpfile.Close()
+				return err
+			}
+
+			if err := tmpfile.Close(); err != nil {
+				return err
+			}
+
+			l.valueFiles[i] = tmpfile.Name()
+			break


prepareStdinValues stops after the first - values file (break). If the user provides multiple -f - entries, later occurrences remain as "-" and will cause helm template to read from stdin after this function already consumed it via io.ReadAll(os.Stdin). Consider either replacing all "-" entries with the same temp file path (read stdin once) or returning an explicit error when more than one stdin values file is provided.

Suggested change

for i, valueFile := range l.valueFiles {

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

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

if err != nil {

return err

}

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 {

_ = tmpfile.Close()

return err

}

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

return err

}

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

break

var (

stdinData []byte

stdinRead bool

stdinTmpFilePath string

)

for i, valueFile := range l.valueFiles {

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

// Read from stdin and create the temp file only once, even if

// "-" appears multiple times in the values files list.

if !stdinRead {

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

if err != nil {

return err

}

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

if err != nil {

return err

}

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

_ = tmpfile.Close()

return err

}

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

return err

}

stdinData = data

stdinTmpFilePath = tmpfile.Name()

stdinRead = true

_ = stdinData // avoid unused variable if not referenced elsewhere

}

l.valueFiles[i] = stdinTmpFilePath

Copilot · 2026-03-30T01:38:49Z

cmd/local.go

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


manifest1/manifest2 can be large. Other commands in this repo set the raw manifest byte slices to nil after manifest.Parse(...) to allow GC to reclaim memory before diff computation. Consider doing the same here after parsing to reduce 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...)

// Allow GC to reclaim memory used by raw manifests before diff computation.

manifest1 = nil

manifest2 = nil

Copilot · 2026-03-30T01:38:49Z

cmd/local_test.go

+func TestLocalCmdExecution(t *testing.T) {
+	tmpDir := t.TempDir()
+	fakeHelm := tmpDir + "/helm"
+	manifestYAML := `---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-config
+  namespace: default
+data:
+  key: value
+`
+
+	err := os.WriteFile(fakeHelm, []byte(`#!/bin/sh
+cat <<EOF
+`+manifestYAML+`
+EOF
+`), 0755)


These tests create a fake Helm binary as a #!/bin/sh script and use Unix-specific assumptions (e.g., /bin/sh, heredocs, POSIX paths). This will fail when running go test on Windows. Consider switching to the existing Go-based fake-helm approach used elsewhere in the repo (e.g., main_test.go's runFakeHelm pattern) or skipping these tests on non-POSIX platforms.

yxxhero requested a review from Copilot October 10, 2025 08:12

Copilot AI reviewed Oct 10, 2025

View reviewed changes

yxxhero requested a review from Copilot November 27, 2025 14:57

Copilot started reviewing on behalf of yxxhero November 27, 2025 14:59 View session

Copilot finished reviewing on behalf of yxxhero November 27, 2025 15:01

Copilot AI reviewed Nov 27, 2025

View reviewed changes

bo0tzz and others added 4 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>

yxxhero force-pushed the feat/diff-local branch from bbbfc81 to cc56dae Compare March 30, 2026 00:05

yxxhero added 3 commits March 30, 2026 08:07

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 March 30, 2026 01:33

Copilot started reviewing on behalf of yxxhero March 30, 2026 01:34 View session

Copilot AI reviewed Mar 30, 2026

View reviewed changes

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

-	cmd := exec.Command(os.Getenv("HELM_BIN"), args...)
+	helmBin := os.Getenv("HELM_BIN")
+	if helmBin == "" {
+		helmBin = "helm"
+	}
+	cmd := exec.Command(helmBin, args...)

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

-	specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)
+	specs2 := manifest.Parse(manifest2, l.namespace, l.normalizeManifests, excludes...)
+	// Allow GC to reclaim memory used by raw manifests before diff computation.
+	manifest1 = nil
+	manifest2 = nil

Conversation

bo0tzz commented Oct 3, 2025

Uh oh!

yxxhero commented Oct 10, 2025

Uh oh!

yxxhero commented Oct 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

bo0tzz commented Oct 10, 2025

Uh oh!

yxxhero commented Oct 11, 2025

Uh oh!

yxxhero commented Nov 27, 2025

Uh oh!

yxxhero commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants