feat: support external diff tool in dry-run mode#2605
Open
p2pdkivenko wants to merge 3 commits intosiderolabs:mainfrom
Open
feat: support external diff tool in dry-run mode#2605p2pdkivenko wants to merge 3 commits intosiderolabs:mainfrom
p2pdkivenko wants to merge 3 commits intosiderolabs:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Member
|
@claude review |
Unix4ever
reviewed
Apr 14, 2026
There was a problem hiding this comment.
Pull request overview
Adds support for rendering diffs during dry-run operations using either a built-in colorized unified diff or an external diff tool configured via OMNI_EXTERNAL_DIFF, and aligns omnictl apply diff computation with the existing diff.Compute algorithm.
Changes:
- Introduce
client/pkg/execdiffto render diffs (built-in) or invoke an external diff tool via temp LIVE/MERGED dirs. - Wire
Differintoomnictl apply --dry-runandomnictl cluster template sync --dry-run. - Remove
diffmatchpatchdependency fromapplyand adjust YAML marshaling to return[]byte.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/pkg/template/operations/sync.go | Adds optional Differ to SyncOptions and routes verbose diff rendering through it. |
| client/pkg/omnictl/cluster/template/sync.go | Instantiates/flushed Differ for dry-run template sync. |
| client/pkg/omnictl/apply.go | Instantiates/flushed Differ for dry-run apply; removes diffmatchpatch; updates verbose output paths. |
| client/pkg/execdiff/execdiff.go | New diff-rendering abstraction (built-in unified diff or external command over temp dirs). |
| client/go.mod | Drops github.com/sergi/go-diff dependency. |
| client/go.sum | Cleans up sums after dependency removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
@p2pdkivenko please address unresolved feedback. If resolved we will review again and if it looks good after the fixes will merge it. Thank you |
Contributor
Author
|
@Unix4ever got it ! |
Contributor
Author
|
@Unix4ever Addressed all the confirmed feedback in 7097292, then merged latest Exit-code contract (
Filename sanitization (path-escape concern Copilot raised on
Verbose-mode diff regression (
Tests (the missing-tests comment)
On the "no temp files" wish — unfortunately every external diff I surveyed ( Tests + |
Member
|
I tested
→ omnictl cluster template diff -f clusters/cluster.yaml -v
Error: unknown shorthand flag: 'v' in -v
Error: unknown shorthand flag: 'v' in -v
→ _out/omnictl-linux-amd64 cluster template diff -f clusters/cluster.yaml
>>> Test, env variable is set: asdfs # checked that it's set.
--- /dev/null
+++ Clusters.omni.sidero.dev(default/example)
@@ -0,0 +1,21 @@
+metadata:
+ namespace: default
+ type: Clusters.omni.sidero.dev
+ id: example
+ version: undefined
+ owner:
+ phase: running
+ created: 2026-04-30T16:17:14+03:00
... |
Add OMNI_EXTERNAL_DIFF environment variable support for `omnictl apply --dry-run` and `omnictl cluster template sync --dry-run`, mirroring kubectl's KUBECTL_EXTERNAL_DIFF behavior. When set, the external diff program is invoked with two temp directory paths containing old (LIVE) and new (MERGED) resource YAML files. Exit codes follow the convention: 0=no diff, 1=has diff, >1=error. When unset, a built-in colorized unified diff is used (replacing the previous diffmatchpatch-based output in apply.go with the better diff.Compute algorithm already used by template sync). Closes siderolabs#2329
- Sanitize resource filenames used in external-diff temp directories to prevent path escape via IDs containing '/', '\\', ':', or '..'. Introduces execdiff.SanitizeFilename and validateFilename enforced on entry; defense-in-depth re-validation at flush time. - Propagate Differ.Flush() hasDiff to command exit status: return the new execdiff.ErrDifferencesFound sentinel from 'omnictl apply' and 'omnictl cluster template sync', map it to exit 1 silently in main; real errors exit 2. Matches documented kubectl-style contract (0 = no diff, 1 = diff, >1 = error). - Restore unified diff output in non-dry-run verbose mode (regression from diffmatchpatch removal): reuse the same diff.Compute-based renderer as the dry-run path so verbose apply is consistent. - Extend template-sync help text with the OMNI_EXTERNAL_DIFF usage and exit-status contract. - Add execdiff unit tests covering sanitize rules, built-in diff rendering, filename validation in external mode, and external-diff exit-code mapping (0 / 1 / >1) via shell stubs. Signed-off-by: Daniil Kivenko <daniil.kivenko@p2p.org>
Two bugs reported on the PR after manual omnictl testing:
1. Error messages were printed twice (once by cobra, once by main):
omnictl cluster template diff -f f.yaml -v
Error: unknown shorthand flag: 'v' in -v
Error: unknown shorthand flag: 'v' in -v
Cobra already prints flag-parse and RunE errors, so main now only
maps ErrDifferencesFound -> exit 1 silently and exits 2 for any
other error. The dry-run RunE handlers set SilenceErrors only when
returning the sentinel, leaving cobra's normal error path intact
for real failures.
2. OMNI_EXTERNAL_DIFF was not honored by 'cluster template diff'
(only by '... sync --dry-run' and 'apply --dry-run'). DiffTemplate
now takes a *execdiff.Differ and routes its output through the
shared renderDiff helper, so the env var works for the diff
subcommand too. Help text updated to document this.
Also restore the missing 'os' import in operations/sync.go that was
lost during the rebase merge with the latest os.Root signature.
p2pdkivenko
force-pushed
the
feat/external-diff-dry-run
branch
from
a99df4d to
b7b9ed5
Compare
Contributor
Author
|
@Unix4ever fixed 👾 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OMNI_EXTERNAL_DIFFenvironment variable support foromnictl apply --dry-runandomnictl cluster template sync --dry-run, mirroring kubectl'sKUBECTL_EXTERNAL_DIFFbehaviorclient/pkg/execdiffpackage that abstracts diff rendering — built-in colorized unified diff or external tool invocation via temp directories (LIVE/MERGED)diffmatchpatchlibrary inapply.gowith the betterdiff.Computealgorithm already used by template syncCloses #2329
Test plan
omnictl apply -f test.yaml --dry-runshows colorized unified diff (built-in mode)OMNI_EXTERNAL_DIFF="diff -u" omnictl apply -f test.yaml --dry-runinvokes external diff with temp dirsOMNI_EXTERNAL_DIFF="colordiff -N -u" omnictl apply -f test.yaml --dry-runworks with argsomnictl cluster template sync -f template.yaml --dry-runworks with both modesgo build ./client/...compilesgo test ./client/...passes