From d9005a00644aea70e37cc72b0997c1f9385134bd Mon Sep 17 00:00:00 2001 From: Eugene Agafonov Date: Thu, 13 Aug 2026 17:57:06 -0700 Subject: [PATCH] feat: add external diff tool support 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. --- README.md | 35 +++++ cmd/options.go | 1 + cmd/options_test.go | 72 +++++++++ diff/diff.go | 25 ++- diff/diff_test.go | 44 +++--- diff/difftool.go | 174 +++++++++++++++++++++ diff/difftool_test.go | 356 ++++++++++++++++++++++++++++++++++++++++++ diff/report.go | 9 +- 8 files changed, 685 insertions(+), 31 deletions(-) create mode 100644 cmd/options_test.go create mode 100644 diff/difftool.go create mode 100644 diff/difftool_test.go diff --git a/README.md b/README.md index 3e4abd9d..38431004 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,7 @@ Flags: -C, --context int output NUM lines of context around changes (default -1) --detailed-exitcode return a non-zero exit code when there are changes --devel use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored. + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments --disable-openapi-validation disables rendered templates validation against the Kubernetes OpenAPI Schema --disable-validation disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install --dry-run string[="client"] --dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation. --dry-run=server enables the cluster access with helm-get and the lookup template function. @@ -219,6 +220,35 @@ helm diff upgrade prod api ./charts/api --output structured When a kind is suppressed via `--suppress`, `changesSuppressed` is set to `true` and field details are omitted. Nested metadata such as labels show the container path (`metadata.labels`) and expose the label key through the `field` property (for example `app.kubernetes.io/version`). +### External diff tool + +Set `--diff-tool` to a command and helm-diff renders the diff with that command instead of its built-in renderers. It writes the old and the new manifests into two temporary files and appends their paths as the last two arguments: + +```shell +# any tool that accepts two file paths works +helm diff upgrade api ./charts/api --diff-tool "diff -u -N" +helm diff upgrade api ./charts/api --diff-tool "difft --language yaml" +helm diff upgrade api ./charts/api --diff-tool "git --no-pager diff --no-index --color" +helm diff upgrade api ./charts/api --diff-tool "delta --side-by-side" +``` + +The command can also be set through the `HELM_DIFF_TOOL` environment variable, which is convenient in a shell profile: + +```shell +export HELM_DIFF_TOOL="difft --language yaml" +helm diff upgrade api ./charts/api +``` + +`--diff-tool` takes precedence over `HELM_DIFF_TOOL`, and either one overrides `--output`. There is no default command: without one, the built-in `--output` renderer is used. + +Notes: + +- 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. +- helm-diff's own exit code is unaffected by the tool: `--detailed-exitcode` still returns `2` based on the changes helm-diff detected. +- `--context`/`-C` is not applied; use the equivalent option of the external tool (for example `diff -U3`). + ## Commands: ### local: @@ -248,6 +278,7 @@ Flags: -a, --api-versions stringArray Kubernetes api versions used for Capabilities.APIVersions -C, --context int output NUM lines of context around changes (default -1) --detailed-exitcode return a non-zero exit code when there are changes + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments --enable-dns enable DNS lookups when rendering templates -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for local @@ -328,6 +359,7 @@ Flags: -C, --context int output NUM lines of context around changes (default -1) --detailed-exitcode return a non-zero exit code when there are changes --devel use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored. + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments --disable-openapi-validation disables rendered templates validation against the Kubernetes OpenAPI Schema --disable-validation disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install --dry-run string[="client"] --dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation. --dry-run=server enables the cluster access with helm-get and the lookup template function. @@ -394,6 +426,7 @@ Usage: Flags: -C, --context int output NUM lines of context around changes (default -1) --detailed-exitcode return a non-zero exit code when there are changes + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for release --include-tests enable the diffing of the helm test hooks @@ -436,6 +469,7 @@ Flags: -C, --context int output NUM lines of context around changes (default -1) --show-secrets-decoded decode secret values in the output --detailed-exitcode return a non-zero exit code when there are changes + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for revision --include-tests enable the diffing of the helm test hooks @@ -472,6 +506,7 @@ Examples: Flags: -C, --context int output NUM lines of context around changes (default -1) --detailed-exitcode return a non-zero exit code when there are changes + --diff-tool string command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments -D, --find-renames float32 Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched -h, --help help for rollback --include-tests enable the diffing of the helm test hooks diff --git a/cmd/options.go b/cmd/options.go index 8733ee98..9be0b65a 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -14,6 +14,7 @@ func AddDiffOptions(f *pflag.FlagSet, o *diff.Options) { f.StringArrayVar(&o.SuppressedKinds, "suppress", []string{}, "allows suppression of the kinds listed in the diff output (can specify multiple, like '--suppress Deployment --suppress Service')") f.IntVarP(&o.OutputContext, "context", "C", -1, "output NUM lines of context around changes") f.StringVar(&o.OutputFormat, "output", "diff", "Possible values: diff, simple, template, json, structured, dyff. When set to \"template\", use the env var HELM_DIFF_TPL to specify the template.") + f.StringVar(&o.DiffToolCommand, "diff-tool", "", "command used to compare the manifests instead of the built-in --output renderers (can also be set via the env var HELM_DIFF_TOOL). The old and the new manifest file paths are appended as the last two arguments") f.BoolVar(&o.StripTrailingCR, "strip-trailing-cr", false, "strip trailing carriage return on input") f.Float32VarP(&o.FindRenames, "find-renames", "D", 0, "Enable rename detection if set to any value greater than 0. If specified, the value denotes the maximum fraction of changed content as lines added + removed compared to total lines in a diff for considering it a rename. Only objects of the same Kind are attempted to be matched") f.StringArrayVar(&o.SuppressedOutputLineRegex, "suppress-output-line-regex", []string{}, "a regex to suppress diff output lines that match") diff --git a/cmd/options_test.go b/cmd/options_test.go new file mode 100644 index 00000000..8011e0c0 --- /dev/null +++ b/cmd/options_test.go @@ -0,0 +1,72 @@ +package cmd + +import ( + "testing" + + "github.com/spf13/pflag" + "github.com/stretchr/testify/require" + + "github.com/databus23/helm-diff/v3/diff" +) + +func processedOptions(t *testing.T, args ...string) diff.Options { + t.Helper() + + var o diff.Options + f := pflag.NewFlagSet("test", pflag.ContinueOnError) + AddDiffOptions(f, &o) + require.NoError(t, f.Parse(args)) + ProcessDiffOptions(f, &o) + + return o +} + +func TestProcessDiffOptionsSuppressSecrets(t *testing.T) { + o := processedOptions(t, "--suppress-secrets") + require.Contains(t, o.SuppressedKinds, "Secret") +} + +func TestAddDiffOptionsHasNoExternalOutputFormat(t *testing.T) { + var o diff.Options + f := pflag.NewFlagSet("test", pflag.ContinueOnError) + AddDiffOptions(f, &o) + + require.NotContains(t, f.Lookup("output").Usage, "external", + "--diff-tool is the only way to select an external tool") +} + +func TestProcessDiffOptionsDiffTool(t *testing.T) { + t.Run("no external diff by default", func(t *testing.T) { + t.Setenv(diff.DiffToolEnvVar, "") + o := processedOptions(t) + require.Equal(t, "diff", o.OutputFormat) + require.Empty(t, o.DiffToolCommand) + require.False(t, o.DiffTool()) + }) + + t.Run("--diff-tool enables the external tool", func(t *testing.T) { + t.Setenv(diff.DiffToolEnvVar, "") + o := processedOptions(t, "--diff-tool", "difft") + require.Equal(t, "difft", o.DiffToolCommand) + require.True(t, o.DiffTool()) + }) + + t.Run("HELM_DIFF_TOOL enables the external tool", func(t *testing.T) { + t.Setenv(diff.DiffToolEnvVar, "colordiff -u") + o := processedOptions(t) + require.True(t, o.DiffTool()) + }) + + t.Run("the external tool overrides an explicit --output", func(t *testing.T) { + t.Setenv(diff.DiffToolEnvVar, "") + o := processedOptions(t, "--diff-tool", "difft", "--output", "json") + require.Equal(t, "json", o.OutputFormat, "--output keeps its value") + require.True(t, o.DiffTool(), "but the external tool takes precedence") + }) + + t.Run("an empty --diff-tool keeps the built-in output", func(t *testing.T) { + t.Setenv(diff.DiffToolEnvVar, "") + o := processedOptions(t, "--diff-tool", "", "--output", "simple") + require.False(t, o.DiffTool()) + }) +} diff --git a/diff/diff.go b/diff/diff.go index 1fc35c73..63c4d1c1 100644 --- a/diff/diff.go +++ b/diff/diff.go @@ -30,13 +30,22 @@ type Options struct { SuppressedKinds []string FindRenames float32 SuppressedOutputLineRegex []string + DiffToolCommand string } const kindSecret = "Secret" -// StructuredOutput returns true when the structured JSON output is requested. +// StructuredOutput returns true when the structured JSON output is requested +// except when using a diff tool, whose input is the line diffs that structured +// output skips. func (o *Options) StructuredOutput() bool { - return o != nil && o.OutputFormat == "structured" + return o != nil && o.OutputFormat == "structured" && !o.DiffTool() +} + +// 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) != "" } type OwnershipDiff struct { @@ -67,8 +76,13 @@ func ManifestReport(oldIndex, newIndex map[string]*manifest.MappingResult, optio } func generateReport(oldIndex, newIndex map[string]*manifest.MappingResult, newOwnedReleases map[string]OwnershipDiff, options *Options) (bool, *Report, error) { - report := Report{findRenames: options.FindRenames} - report.setupReportFormat(options.OutputFormat) + report := Report{findRenames: options.FindRenames, diffToolCommand: options.DiffToolCommand} + if options.DiffTool() { + // A configured diff tool replaces whatever built-in output was selected. + setupDiffToolReport(&report) + } else { + report.setupReportFormat(options.OutputFormat) + } var possiblyRemoved []string for name, diff := range newOwnedReleases { @@ -121,7 +135,8 @@ func doSuppress(report Report, suppressedOutputLineRegex []string) (Report, erro } filteredReport := Report{ - findRenames: report.findRenames, + findRenames: report.findRenames, + diffToolCommand: report.diffToolCommand, } filteredReport.format = report.format filteredReport.Entries = []ReportEntry{} diff --git a/diff/diff_test.go b/diff/diff_test.go index 0a9e3ea6..8d783899 100644 --- a/diff/diff_test.go +++ b/diff/diff_test.go @@ -278,7 +278,7 @@ annotations: t.Run("OnChange", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -297,7 +297,7 @@ annotations: t.Run("OnChangeWithSuppress", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{"apiVersion"}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{"apiVersion"}, ""} if changesSeen := Manifests(specBeta, specReleaseSpec, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -316,7 +316,7 @@ annotations: t.Run("OnChangeWithSuppressAll", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{"apiVersion"}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{"apiVersion"}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -328,7 +328,7 @@ annotations: t.Run("OnChangeRename", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} if changesSeen := Manifests(specReleaseSpec, specReleaseRenamed, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -349,7 +349,7 @@ annotations: t.Run("OnChangeRenameAndUpdate", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} if changesSeen := Manifests(specReleaseSpec, specReleaseRenamedAndUpdated, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -371,7 +371,7 @@ annotations: t.Run("OnChangeRenameAndAdded", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} if changesSeen := Manifests(specReleaseSpec, specReleaseRenamedAndAdded, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -395,7 +395,7 @@ annotations: t.Run("OnChangeRenameAndAddedWithPartialSuppress", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{"app: "}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{"app: "}, ""} if changesSeen := Manifests(specReleaseSpec, specReleaseRenamedAndAdded, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -418,7 +418,7 @@ annotations: t.Run("OnChangeRenameAndRemoved", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} if changesSeen := Manifests(specReleaseRenamedAndAdded, specReleaseSpec, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -442,7 +442,7 @@ annotations: t.Run("OnChangeRenameAndRemovedWithPartialSuppress", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{"app: "}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{"app: "}, ""} if changesSeen := Manifests(specReleaseRenamedAndAdded, specReleaseSpec, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -465,7 +465,7 @@ annotations: t.Run("OnNoChange", func(t *testing.T) { var buf2 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`") @@ -476,7 +476,7 @@ annotations: t.Run("OnChangeRemoved", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} if changesSeen := Manifests(specRelease, nil, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -494,7 +494,7 @@ annotations: t.Run("OnChangeRemovedWithResourcePolicyKeep", func(t *testing.T) { var buf2 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specReleaseKeep, nil, &diffOptions, &buf2); changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`") @@ -505,7 +505,7 @@ annotations: t.Run("OnChangeSimple", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"simple", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"simple", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -518,7 +518,7 @@ Plan: 0 to add, 1 to change, 0 to destroy, 0 to change ownership. t.Run("OnNoChangeSimple", func(t *testing.T) { var buf2 bytes.Buffer - diffOptions := Options{"simple", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"simple", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`") } @@ -528,7 +528,7 @@ Plan: 0 to add, 1 to change, 0 to destroy, 0 to change ownership. t.Run("OnChangeTemplate", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -546,7 +546,7 @@ Plan: 0 to add, 1 to change, 0 to destroy, 0 to change ownership. t.Run("OnChangeJSON", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"json", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"json", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -564,7 +564,7 @@ Plan: 0 to add, 1 to change, 0 to destroy, 0 to change ownership. t.Run("OnNoChangeTemplate", func(t *testing.T) { var buf2 bytes.Buffer - diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specRelease, specRelease, &diffOptions, &buf2); changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`") @@ -576,7 +576,7 @@ Plan: 0 to add, 1 to change, 0 to destroy, 0 to change ownership. t.Run("OnChangeCustomTemplate", func(t *testing.T) { var buf1 bytes.Buffer os.Setenv("HELM_DIFF_TPL", "testdata/customTemplate.tpl") - diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}} + diffOptions := Options{"template", 10, false, true, false, []string{}, 0.0, []string{}, ""} if changesSeen := Manifests(specBeta, specRelease, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`") @@ -1134,7 +1134,7 @@ stringData: t.Run("OnChangeSecretWithByteData", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, false, false, []string{}, 0.5, []string{}} // NOTE: ShowSecrets = false + diffOptions := Options{"diff", 10, false, false, false, []string{}, 0.5, []string{}, ""} // NOTE: ShowSecrets = false if changesSeen := Manifests(specSecretWithByteData, specSecretWithByteDataChanged, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -1159,7 +1159,7 @@ stringData: t.Run("OnChangeSecretWithStringData", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, false, false, []string{}, 0.5, []string{}} // NOTE: ShowSecrets = false + diffOptions := Options{"diff", 10, false, false, false, []string{}, 0.5, []string{}, ""} // NOTE: ShowSecrets = false if changesSeen := Manifests(specSecretWithStringData, specSecretWithStringDataChanged, &diffOptions, &buf1); !changesSeen { t.Error("Unexpected return value from Manifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`") @@ -1284,7 +1284,7 @@ data: t.Run("OnChangeOwnershipWithoutSpecChange", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} // NOTE: ShowSecrets = false + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} // NOTE: ShowSecrets = false newOwnedReleases := map[string]OwnershipDiff{ "default, foobar, ConfigMap (v1)": { @@ -1304,7 +1304,7 @@ data: t.Run("OnChangeOwnershipWithSpecChange", func(t *testing.T) { var buf1 bytes.Buffer - diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}} // NOTE: ShowSecrets = false + diffOptions := Options{"diff", 10, false, true, false, []string{}, 0.5, []string{}, ""} // NOTE: ShowSecrets = false specNew := map[string]*manifest.MappingResult{ "default, foobar, ConfigMap (v1)": { diff --git a/diff/difftool.go b/diff/difftool.go new file mode 100644 index 00000000..2862e5ea --- /dev/null +++ b/diff/difftool.go @@ -0,0 +1,174 @@ +package diff + +import ( + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/aryann/difflib" +) + +const ( + // DiffToolEnvVar holds the diff tool command line when no flag is given. + // Setting it is by itself a request to render the diff with that command. + DiffToolEnvVar = "HELM_DIFF_TOOL" +) + +// diffToolCommand returns the configured command, falling back to the +// environment variable. There is deliberately no default: helm-diff never picks a +// diff tool on the user's behalf, so an unset command is a configuration error +// rather than an invitation to guess. +func diffToolCommand(configured string) string { + if strings.TrimSpace(configured) != "" { + return configured + } + + return strings.TrimSpace(os.Getenv(DiffToolEnvVar)) +} + +// splitDiffToolCommand splits a command line into a command and its arguments. +// Whitespace separates arguments unless quoted, so that paths containing spaces can +// be expressed as `"/opt/my tools/diff" -u`. The result is executed directly rather +// than through a shell, so the generated file paths cannot be expanded or injected. +func splitDiffToolCommand(command string) []string { + var ( + args []string + current strings.Builder + quote rune + started bool + ) + + for _, r := range command { + switch { + case quote != 0: + if r == quote { + quote = 0 + } else { + current.WriteRune(r) + } + case r == '\'' || r == '"': + quote = r + started = true + case r == ' ' || r == '\t' || r == '\n' || r == '\r': + if started { + args = append(args, current.String()) + current.Reset() + started = false + } + default: + current.WriteRune(r) + started = true + } + } + + if started { + args = append(args, current.String()) + } + + return args +} + +func setupDiffToolReport(r *Report) { + r.format.output = printDiffToolReport +} + +// printDiffToolReport writes both sides of the report to temporary files and +// appends their paths as the last two arguments of the diff tool command, +// streaming its output to `to`. +func printDiffToolReport(r *Report, to io.Writer) { + if len(r.Entries) == 0 { + return + } + + args := splitDiffToolCommand(diffToolCommand(r.diffToolCommand)) + if len(args) == 0 { + // Unreachable: this printer is only installed once a command is configured. + fmt.Fprintf(os.Stderr, "Error: no diff tool configured\n") + return + } + + oldFile, newFile, cleanup, err := createDiffToolFiles() + if err != nil { + fmt.Fprintf(os.Stderr, "Error: unable to create temporary files for the diff tool: %v\n", err) + return + } + defer cleanup() + + if err := writeDiffToolSides(r, oldFile, newFile); err != nil { + fmt.Fprintf(os.Stderr, "Error: unable to write manifests for the diff tool: %v\n", err) + return + } + + cmd := exec.Command(args[0], append(args[1:], oldFile, newFile)...) + cmd.Stdout = to + cmd.Stderr = os.Stderr + + // Exit code 1 conventionally means "differences found", the expected case here. + // Other failures are reported but must not abort helm-diff, whose own exit code + // is derived from the report. + if err := cmd.Run(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return + } + fmt.Fprintf(os.Stderr, "Error: diff tool %q failed: %v\n", strings.Join(args, " "), err) + } +} + +func createDiffToolFiles() (oldFile, newFile string, cleanup func(), err error) { + // Stable basenames in a private directory: diff tools label their output + // with the file names, which randomized temp names would make unreadable. + dir, err := os.MkdirTemp("", "helm-diff-tool") + if err != nil { + return "", "", nil, err + } + + return filepath.Join(dir, "current.yaml"), + filepath.Join(dir, "new.yaml"), + func() { _ = os.RemoveAll(dir) }, + nil +} + +// 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 { + var current, next strings.Builder + + for _, entry := range r.Entries { + header := "---\n# Source: " + entry.Key + "\n" + _, _ = current.WriteString(header) + _, _ = next.WriteString(header) + + if containsKind(entry.SuppressedKinds, entry.Kind) { + // Identical placeholder on both sides: the tool must report no change + // rather than receive the suppressed content. + placeholder := fmt.Sprintf("# Changes suppressed on sensitive content of type %s\n", entry.Kind) + _, _ = current.WriteString(placeholder) + _, _ = next.WriteString(placeholder) + continue + } + + for _, record := range entry.Diffs { + switch record.Delta { + case difflib.Common: + _, _ = current.WriteString(record.Payload + "\n") + _, _ = next.WriteString(record.Payload + "\n") + case difflib.LeftOnly: + _, _ = current.WriteString(record.Payload + "\n") + case difflib.RightOnly: + _, _ = next.WriteString(record.Payload + "\n") + } + } + } + + if err := os.WriteFile(oldPath, []byte(current.String()), 0o600); err != nil { + return err + } + + return os.WriteFile(newPath, []byte(next.String()), 0o600) +} diff --git a/diff/difftool_test.go b/diff/difftool_test.go new file mode 100644 index 00000000..a0abf1e7 --- /dev/null +++ b/diff/difftool_test.go @@ -0,0 +1,356 @@ +package diff + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/aryann/difflib" + "github.com/stretchr/testify/require" + + "github.com/databus23/helm-diff/v3/manifest" +) + +func TestSplitDiffToolCommand(t *testing.T) { + tests := []struct { + name string + command string + expected []string + }{ + { + name: "empty", + command: " ", + expected: nil, + }, + { + name: "simple", + command: "diff -u -N", + expected: []string{"diff", "-u", "-N"}, + }, + { + name: "collapses repeated whitespace", + command: " diff \t -u ", + expected: []string{"diff", "-u"}, + }, + { + name: "double quoted argument keeps spaces", + command: `"/opt/my tools/diff" --color=always`, + expected: []string{"/opt/my tools/diff", "--color=always"}, + }, + { + name: "single quoted argument keeps spaces", + command: `'/opt/my tools/diff' -u`, + expected: []string{"/opt/my tools/diff", "-u"}, + }, + { + name: "quotes inside an argument", + command: `git --no-pager diff --src-prefix="a b/"`, + expected: []string{"git", "--no-pager", "diff", "--src-prefix=a b/"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, splitDiffToolCommand(tt.command)) + }) + } +} + +func TestDiffToolCommandResolution(t *testing.T) { + t.Run("empty when nothing is configured", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "") + require.Empty(t, diffToolCommand("")) + }) + + t.Run("environment variable is used when no command is given", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "colordiff -u") + require.Equal(t, "colordiff -u", diffToolCommand("")) + }) + + t.Run("explicit command wins over the environment variable", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "colordiff -u") + require.Equal(t, "difft", diffToolCommand("difft")) + }) +} + +func TestWriteDiffToolSides(t *testing.T) { + report := &Report{ + Entries: []ReportEntry{ + { + Key: "default, nginx, Deployment (apps)", + Kind: "Deployment", + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{ + {Payload: "kind: Deployment", Delta: difflib.Common}, + {Payload: " replicas: 2", Delta: difflib.LeftOnly}, + {Payload: " replicas: 3", Delta: difflib.RightOnly}, + }, + }, + }, + } + + dir := t.TempDir() + oldPath := filepath.Join(dir, "old") + newPath := filepath.Join(dir, "new") + require.NoError(t, writeDiffToolSides(report, oldPath, newPath)) + + oldContent, err := os.ReadFile(oldPath) + require.NoError(t, err) + newContent, err := os.ReadFile(newPath) + require.NoError(t, err) + + require.Equal(t, "---\n# Source: default, nginx, Deployment (apps)\nkind: Deployment\n replicas: 2\n", string(oldContent)) + require.Equal(t, "---\n# Source: default, nginx, Deployment (apps)\nkind: Deployment\n replicas: 3\n", string(newContent)) +} + +func TestWriteDiffToolSidesSuppressedKind(t *testing.T) { + report := &Report{ + Entries: []ReportEntry{ + { + Key: "default, mysecret, Secret (v1)", + Kind: "Secret", + SuppressedKinds: []string{"Secret"}, + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{ + {Payload: "kind: Secret", Delta: difflib.Common}, + {Payload: " password: aGkK", Delta: difflib.LeftOnly}, + {Payload: " password: Ynll", Delta: difflib.RightOnly}, + }, + }, + }, + } + + dir := t.TempDir() + oldPath := filepath.Join(dir, "old") + newPath := filepath.Join(dir, "new") + require.NoError(t, writeDiffToolSides(report, oldPath, newPath)) + + oldContent, err := os.ReadFile(oldPath) + require.NoError(t, err) + newContent, err := os.ReadFile(newPath) + require.NoError(t, err) + + require.NotContains(t, string(oldContent), "aGkK", "suppressed kinds must not leak their content") + require.NotContains(t, string(newContent), "Ynll", "suppressed kinds must not leak their content") + require.Contains(t, string(oldContent), "Changes suppressed on sensitive content of type Secret") + require.Equal(t, string(oldContent), string(newContent), "suppressed entries must be identical on both sides") +} + +func TestPrintDiffToolReport(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("relies on a POSIX diff implementation") + } + + report := &Report{ + diffToolCommand: "diff -u -N", + Entries: []ReportEntry{ + { + Key: "default, nginx, Deployment (apps)", + Kind: "Deployment", + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{ + {Payload: "kind: Deployment", Delta: difflib.Common}, + {Payload: " replicas: 2", Delta: difflib.LeftOnly}, + {Payload: " replicas: 3", Delta: difflib.RightOnly}, + }, + }, + }, + } + + var buf bytes.Buffer + printDiffToolReport(report, &buf) + + output := buf.String() + require.Contains(t, output, "- replicas: 2") + require.Contains(t, output, "+ replicas: 3") +} + +func TestPrintDiffToolReportEmpty(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("relies on a POSIX diff implementation") + } + + report := &Report{diffToolCommand: "diff -u -N", Entries: []ReportEntry{}} + + var buf bytes.Buffer + printDiffToolReport(report, &buf) + + require.Empty(t, buf.String()) +} + +func TestPrintDiffToolReportPassesBothFiles(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("relies on a POSIX shell") + } + + report := &Report{ + diffToolCommand: "echo", + Entries: []ReportEntry{ + { + Key: "default, nginx, Deployment (apps)", + Kind: "Deployment", + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{{Payload: "kind: Deployment", Delta: difflib.Common}}, + }, + }, + } + + var buf bytes.Buffer + printDiffToolReport(report, &buf) + + fields := bytes.Fields(buf.Bytes()) + require.Len(t, fields, 2, "the external command must receive exactly the two file paths") + require.NotEqual(t, string(fields[0]), string(fields[1]), "both sides must be distinct files") +} + +func TestManifestsDiffToolOutput(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("relies on a POSIX diff implementation") + } + t.Setenv(DiffToolEnvVar, "") + + old := map[string]*manifest.MappingResult{ + "default, nginx, Deployment (apps)": { + Name: "default, nginx, Deployment (apps)", + Kind: "Deployment", + Content: "kind: Deployment\nspec:\n replicas: 2\n", + }, + } + updated := map[string]*manifest.MappingResult{ + "default, nginx, Deployment (apps)": { + Name: "default, nginx, Deployment (apps)", + Kind: "Deployment", + Content: "kind: Deployment\nspec:\n replicas: 3\n", + }, + } + + t.Run("the command alone selects the external tool", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff", DiffToolCommand: "diff -u -N"} + + require.True(t, Manifests(old, updated, opts, &buf)) + require.Contains(t, buf.String(), "- replicas: 2") + require.Contains(t, buf.String(), "+ replicas: 3") + require.Contains(t, buf.String(), "@@", "expected unified diff output from the external tool") + }) + + t.Run("the environment variable alone selects the external tool", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "diff -u -N") + var buf bytes.Buffer + opts := &Options{OutputFormat: "diff"} + + require.True(t, Manifests(old, updated, opts, &buf)) + require.Contains(t, buf.String(), "@@") + }) + + t.Run("the external tool wins over every built-in output", func(t *testing.T) { + for _, format := range []string{"diff", "simple", "json", "template", "structured", "dyff"} { + var buf bytes.Buffer + opts := &Options{OutputFormat: format, DiffToolCommand: "diff -u -N"} + + require.True(t, Manifests(old, updated, opts, &buf)) + require.Contains(t, buf.String(), "@@", + "output %q must be overridden by the external diff command", format) + } + }) + + t.Run("built-in output is used when no command is configured", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{OutputFormat: "simple"} + + require.True(t, Manifests(old, updated, opts, &buf)) + require.Contains(t, buf.String(), "to be changed.") + require.NotContains(t, buf.String(), "@@") + }) + + t.Run("honors suppress-output-line-regex", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{ + DiffToolCommand: "diff -u -N", + SuppressedOutputLineRegex: []string{"replicas"}, + } + + Manifests(old, updated, opts, &buf) + require.NotContains(t, buf.String(), "replicas") + }) + + t.Run("no output when there are no changes", func(t *testing.T) { + var buf bytes.Buffer + opts := &Options{DiffToolCommand: "diff -u -N"} + + require.False(t, Manifests(old, old, opts, &buf)) + require.Empty(t, buf.String()) + }) +} + +func TestDiffToolEnabled(t *testing.T) { + t.Run("disabled when nothing is configured", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "") + require.False(t, (&Options{OutputFormat: "diff"}).DiffTool()) + }) + + t.Run("enabled by the command", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "") + require.True(t, (&Options{DiffToolCommand: "difft"}).DiffTool()) + }) + + t.Run("enabled by the environment variable", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "difft") + require.True(t, (&Options{}).DiffTool()) + }) + + t.Run("a blank command does not enable it", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, " ") + require.False(t, (&Options{DiffToolCommand: " "}).DiffTool()) + }) + + t.Run("structured output is disabled while an external tool is used", func(t *testing.T) { + t.Setenv(DiffToolEnvVar, "") + opts := &Options{OutputFormat: "structured", DiffToolCommand: "difft"} + require.True(t, opts.DiffTool()) + require.False(t, opts.StructuredOutput(), + "the external tool needs the line diffs that structured output skips") + }) +} + +func TestPrintDiffToolReportNoCommand(t *testing.T) { + t.Setenv(DiffToolEnvVar, "") + + report := &Report{ + Entries: []ReportEntry{ + { + Key: "default, nginx, Deployment (apps)", + Kind: "Deployment", + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{{Payload: "kind: Deployment", Delta: difflib.Common}}, + }, + }, + } + + var buf bytes.Buffer + require.NotPanics(t, func() { printDiffToolReport(report, &buf) }) + require.Empty(t, buf.String(), "without a command there is nothing to render") +} + +func TestPrintDiffToolReportCommandFailure(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("relies on a POSIX shell") + } + + report := &Report{ + diffToolCommand: "helm-diff-no-such-external-tool", + Entries: []ReportEntry{ + { + Key: "default, nginx, Deployment (apps)", + Kind: "Deployment", + ChangeType: "MODIFY", + Diffs: []difflib.DiffRecord{{Payload: "kind: Deployment", Delta: difflib.Common}}, + }, + }, + } + + var buf bytes.Buffer + require.NotPanics(t, func() { printDiffToolReport(report, &buf) }) +} diff --git a/diff/report.go b/diff/report.go index 7a2692d4..f23be6b6 100644 --- a/diff/report.go +++ b/diff/report.go @@ -20,10 +20,11 @@ import ( // Report to store report data and format type Report struct { - format ReportFormat - Entries []ReportEntry - mode string - findRenames float32 + format ReportFormat + Entries []ReportEntry + mode string + findRenames float32 + diffToolCommand string } // ReportEntry to store changes between releases