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