Add external diff tool support - #1048
Conversation
There was a problem hiding this comment.
Pull request overview
Adds external diff-tool support using a CLI flag or environment variable.
Changes:
- Adds
--diff-toolandHELM_DIFF_TOOL. - Reconstructs redacted/suppressed manifests in temporary files.
- Adds documentation and unit tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents external diff usage. |
cmd/options.go |
Registers the new flag. |
cmd/options_test.go |
Tests option processing. |
diff/diff.go |
Selects external rendering. |
diff/diff_test.go |
Updates option fixtures. |
diff/difftool.go |
Executes external tools. |
diff/difftool_test.go |
Tests external diff behavior. |
diff/report.go |
Stores the configured command. |
Suppressed comments (1)
README.md:239
- This example also supplies three positional arguments even though
upgradeaccepts exactly a release and chart, so it cannot demonstrate the environment variable. Passprodthrough--namespaceinstead.
helm diff upgrade prod api ./charts/api
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
875e600 to
d9005a0
Compare
|
@eagafonov please fix ci issue. |
Add --diff-tool flag and HELM_DIFF_TOOL env var to render diffs with an external command. Manifests are written to two temp files whose paths are appended as the last two arguments. Secret redaction and line suppression stay in effect since manifests are reconstructed from the report entries. Exit code 1 (differences found) is ignored; other failures report to stderr without aborting helm-diff.
Replace repeated "structured"/"MODIFY" and related string literals in diff.go, report.go and structured.go with named constants, resolving the goconst lint failures. No behavior change.
d9005a0 to
c8058bf
Compare
Fixed in a separate commit as a uniform refactor Here is weird thing: my 'diff tool' patch hasn't introduced the issue, but somehow reviled it. |
yxxhero
left a comment
yxxhero
left a comment
There was a problem hiding this comment.
Thanks for this PR — overall it's a well-executed feature and the code quality is above the bar for this repo. The core design decision — reconstructing the two file sides from the report entries rather than writing the raw manifests — means secret redaction, --suppress, --suppress-output-line-regex, --strip-trailing-cr and rename detection all keep working with zero duplicated filtering logic.
Also appreciated:
- Overriding the output printer at the report layer (instead of if/else in
cmd), and correctly carryingdiffToolCommandthroughdoSuppresswhen the report is rebuilt — an easy detail to miss. - Security posture: no shell involved (manifest content can never reach argv), 0700 private dir + 0600 files +
RemoveAllcleanup — notably stricter than the existingprintDyffReport, which ignores all temp-file errors. - Exit-code semantics: the tool's
1("differences found") is ignored while helm-diff's own exit code (incl.--detailed-exitcode= 2) stays driven by the report — and it's documented. - Thorough README (precedence, no default command,
--contextnot applied, no shell features → wrapper script) and good tests, including all six--outputformats being overridden.
I'd be happy to approve once the points below are discussed — #1 and #2 look worth fixing before merge; #3/#4 at minimum deserve a README note.
Main issues
HELM_DIFF_TOOLsilently overrides an explicit--output— a footgun for CI consumers (inline ondiff/diff.go).--suppress-output-line-regexleaves no trace in tool mode — user sees exit code 2 with an empty diff (inline ondiff/difftool.go).- ADD/REMOVE/OWNERSHIP semantics are invisible to the tool user — suggest writing the change type into the header comment.
- GUI tools that return immediately race with
defer cleanup()— and GUI tools are the primary use case from #638.
Minor
current.yamlpairs withnew.yamlbut is returned asoldFile— naming is asymmetric; the manifest content already carries its own# Source:header, so the injected one slightly duplicates it.- An unclosed quote in the command is silently dropped (
diff -u "foo→diff -u foo). - The constants refactor is incomplete:
outputFormatDiffis missing and unknown formats still fall back silently (pre-existing behavior, not introduced here). TestAddDiffOptionsHasNoExternalOutputFormatasserts a help string doesn't contain "external" — brittle with no forward value.- All non-trivial tests skip on
windows-latest, which is in the CI matrix. - No timeout around the external command — a hung tool hangs helm-diff (consistent with kubectl, fine, maybe worth a README line).
- Test gaps: exit-code-1 message content, end-to-end Secret redaction reaching the temp files, the
MODIFY_SUPPRESSEDintegration path, and--suppressend-to-end (currently only covered at thewriteDiffToolSidesunit level).
| // DiffTool reports whether the diff is rendered by an external tool. Configuring a | ||
| // command is the only way to ask for it, and it overrides the built-in outputs. | ||
| func (o *Options) DiffTool() bool { | ||
| return o != nil && diffToolCommand(o.DiffToolCommand) != "" |
There was a problem hiding this comment.
Main issue #1: env var silently overrides an explicit --output.
Since the README recommends putting export HELM_DIFF_TOOL=... in the shell profile, anyone on that machine running a CI script with helm diff ... --output json (whose stdout is parsed by a downstream tool) will silently get free-form text from the external tool instead, breaking the script in a way that's hard to debug.
Suggestion: in ProcessDiffOptions (which has the FlagSet, so f.Changed("output") is available), let an explicitly-set --output win over the env var — or at least print a warning to stderr. The precedence chain explicit flag > explicit env > default is safer for unattended use.
| // writeDiffToolSides reconstructs the old and the new manifests from the report | ||
| // entries rather than from the raw manifests, which keeps secret redaction and line | ||
| // suppression in effect for whatever the diff tool receives. | ||
| func writeDiffToolSides(r *Report, oldPath, newPath string) error { |
There was a problem hiding this comment.
Main issue #2: --suppress-output-line-regex leaves no trace in tool mode.
When every changed line of a MODIFY entry is filtered out, doSuppress flips the entry to MODIFY_SUPPRESSED with empty Diffs (diff.go:196-200). writeDiffToolSides then writes only the header on both sides → the external tool reports no differences, yet helm-diff still exits 2 (entries exist and seenAnyChanges is computed before suppression). The user gets the contradictory combination of "exit code 2 but empty diff". The built-in outputs handle this by printing "has changed, but diff is empty after suppression."
Note the --suppress (SuppressedKinds) path right below does this correctly — identical placeholder on both sides. Suggest doing the same for MODIFY_SUPPRESSED (and for an ADD whose lines were all filtered out), e.g. # Changes suppressed by --suppress-output-line-regex.
| fmt.Fprintf(os.Stderr, "Error: unable to create temporary files for the diff tool: %v\n", err) | ||
| return | ||
| } | ||
| defer cleanup() |
There was a problem hiding this comment.
Main issue #4: GUI tools that return immediately race with defer cleanup().
Tools like code --diff (VS Code returns immediately) or daemonized viewers will have the temp dir removed by RemoveAll right after cmd.Run() returns, leaving an empty diff panel. Since GUI diff viewers (Meld, VS Code — see #638) are the primary motivation for this feature, this deserves at least a README note: the command must block until it's done reading the files, otherwise wrap it in a script that waits.
| return "", "", nil, err | ||
| } | ||
|
|
||
| return filepath.Join(dir, "current.yaml"), |
There was a problem hiding this comment.
Nit: current.yaml is returned as oldFile but doesn't pair with new.yaml — old.yaml/new.yaml would read better, since tools label their hunks/panels with these file names (that's the stated reason for stable basenames, and it works nicely).
Also, manifest Content already begins with its own ---\n# Source: <template path> header, so the injected # Source: <key> slightly duplicates it for regular entries — only OWNERSHIP entries genuinely need the injected one. Not blocking, just something to consider.
| args = append(args, current.String()) | ||
| } | ||
|
|
||
| return args |
There was a problem hiding this comment.
Minor: an unclosed quote is silently swallowed — splitDiffToolCommand("diff -u \"foo") yields ["diff", "-u", "foo"] with the quote just gone, which will confuse users. Consider returning an error from the tokenizer. No \" escaping either — acceptable given the README already points complex cases at a wrapper script.
| require.Contains(t, o.SuppressedKinds, "Secret") | ||
| } | ||
|
|
||
| func TestAddDiffOptionsHasNoExternalOutputFormat(t *testing.T) { |
There was a problem hiding this comment.
This test asserts the --output usage string does not contain the word "external" — it's brittle (a future, perfectly reasonable mention of --diff-tool in that usage text would break it) and doesn't verify any actual behavior. I'd drop it or turn it into a positive assertion.
|
|
||
| - The command is executed directly, not through a shell, so pipes and shell expansion are not available. Wrap arguments containing spaces in quotes, for example `--diff-tool '"/opt/my tools/diff" -u'`. For anything more involved, point the flag at a wrapper script. | ||
| - The manifests handed to the tool are the ones from the diff report, so `--suppress`, `--suppress-output-line-regex` and secret redaction still apply. Secrets are redacted unless `--show-secrets` is given, and suppressed kinds are replaced by a placeholder on both sides. | ||
| - An exit code of `1` from the tool is treated as "differences found" and ignored. Other failures are reported on stderr without aborting helm-diff. |
There was a problem hiding this comment.
Two suggestions for this Notes list, related to the main review points:
- Note that ADD/REMOVE/OWNERSHIP change-type annotations from the built-in output are not visible when using an external tool — the tool only sees file content. A cheap improvement on the code side would be writing the change type into the header comment (e.g.
# Change: ADD), which diff tools render as context lines. - Note that the command must block until it has finished reading the files — GUI tools that return immediately (e.g.
code --diff) will see the temp files deleted before they display them.
Add --diff-tool flag and HELM_DIFF_TOOL env var to render diffs with an external command.
Manifests are written to two temp files whose paths are appended as the last two arguments.
Secret redaction and line suppression stay in effect since manifests are reconstructed from the report entries.
Exit code 1 (differences found) is ignored; other failures report to stderr without aborting helm-diff.
The patch addresses #638