fix: make --find-renames work with --output=dyff by yxxhero · Pull Request #961 · databus23/helm-diff

yxxhero · 2026-03-24T01:39:46Z

Summary

Remove dyff.DetectRenames(true) from dyff output format to allow helm-diff's --find-renames option to work correctly
Fix tautological condition warning in doDiff function
Add unit tests for dyff report output

Fixes #632

Copilot

Pull request overview

Fixes dyff output behavior so helm-diff’s --find-renames option can control rename detection (instead of dyff doing its own rename pairing), and adds/updates tests around dyff report generation.

Changes:

Disable dyff’s internal rename detection in printDyffReport to restore --find-renames semantics for --output=dyff.
Simplify doDiff by removing a tautological oldContent != nil check in the remove case.
Add unit tests covering basic dyff report output paths.

Reviewed changes

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

File	Description
diff/report.go	Removes `dyff.DetectRenames(true)` by switching to an explicit options slice passed into dyff comparison.
diff/diff.go	Simplifies remove-case logic in `doDiff` by eliminating redundant nil checks.
diff/report_test.go	Adds unit tests exercising `printDyffReport` output generation.

Comments suppressed due to low confidence (1)

diff/report.go:124

dyff.CompareInputFiles and reportWriter.WriteReport errors are being ignored. If parsing/comparison fails, report may be nil and WriteReport can panic or silently emit incomplete output. Please handle these errors (e.g., write an error message to to and return, or change printDyffReport to return an error and propagate it).

	var compareOptions []dyff.CompareOption
	compareOptions = append(compareOptions,
		dyff.IgnoreWhitespaceChanges(true),
		dyff.KubernetesEntityDetection(true),
	)
	report, _ := dyff.CompareInputFiles(currentInputFile, newInputFile, compareOptions...)
	reportWriter := &dyff.HumanReport{
		Report:               report,
		OmitHeader:           true,
		MinorChangeThreshold: 0.1,
	}
	_ = reportWriter.WriteReport(to)

Copilot · 2026-03-24T01:49:41Z

diff/report_test.go

+func TestPrintDyffReportWithAddAndRemove(t *testing.T) {
+	report := &Report{
+		Entries: []ReportEntry{
+			{
+				Key:        "default, old-app, Deployment (apps)",
+				Kind:       "Deployment",
+				ChangeType: "REMOVE",
+				Diffs: []difflib.DiffRecord{
+					{Payload: "apiVersion: apps/v1", Delta: difflib.LeftOnly},
+					{Payload: "kind: Deployment", Delta: difflib.LeftOnly},
+					{Payload: "metadata:", Delta: difflib.LeftOnly},
+					{Payload: "  name: old-app", Delta: difflib.LeftOnly},
+				},
+			},
+			{
+				Key:        "default, new-app, Deployment (apps)",
+				Kind:       "Deployment",
+				ChangeType: "ADD",
+				Diffs: []difflib.DiffRecord{
+					{Payload: "apiVersion: apps/v1", Delta: difflib.RightOnly},
+					{Payload: "kind: Deployment", Delta: difflib.RightOnly},
+					{Payload: "metadata:", Delta: difflib.RightOnly},
+					{Payload: "  name: new-app", Delta: difflib.RightOnly},
+				},
+			},
+		},
+	}
+
+	var buf bytes.Buffer
+	printDyffReport(report, &buf)
+
+	output := buf.String()
+	require.NotEmpty(t, output)
+}


The new dyff tests don't assert the regression fix described in #632. In particular, TestPrintDyffReportWithAddAndRemove only checks non-empty output, so it would pass even if dyff still reorders/pairs removed+added resources (the original bug). Please strengthen this test (or add another) to assert that add/remove output is distinguishable from a rename-as-modify scenario (e.g., compare outputs for REMOVE+ADD vs a single MODIFY entry and assert they differ / contain expected identifiers).

Copilot · 2026-03-24T01:49:42Z

diff/report_test.go

+	require.True(t, strings.Contains(output, "replicas") || strings.Contains(output, "2") || strings.Contains(output, "3"),
+		"Expected dyff output to contain information about replicas change")


require.True(strings.Contains(output, "replicas") || strings.Contains(output, "2") || strings.Contains(output, "3")) is very permissive and can pass even if the dyff output isn't actually describing the replicas change (e.g., if some unrelated "2" appears). Please tighten the assertion to check for a more specific/robust signal of the replicas field change (for example requiring the field name and at least one of the expected values).

Suggested change

require.True(t, strings.Contains(output, "replicas") || strings.Contains(output, "2") || strings.Contains(output, "3"),

"Expected dyff output to contain information about replicas change")

require.Contains(t, output, "replicas: 2", "Expected dyff output to show original replicas value")

require.Contains(t, output, "replicas: 3", "Expected dyff output to show updated replicas value")

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-24T03:53:13Z

diff/report_test.go

+					{Payload: "- name: old-app", Delta: difflib.LeftOnly},
+					{Payload: "+ name: new-app", Delta: difflib.RightOnly},


In modifyReport, the DiffRecord payloads include diff markers (- / + ). difflib.Diff payloads are raw input lines (the delta encodes left/right), so these prefixes make the YAML content here unrepresentative and can change YAML structure compared to real manifests. Consider using the actual YAML lines (with consistent indentation) and rely on Delta to indicate removal/addition.

Suggested change

{Payload: "- name: old-app", Delta: difflib.LeftOnly},

{Payload: "+ name: new-app", Delta: difflib.RightOnly},

{Payload: " name: old-app", Delta: difflib.LeftOnly},

{Payload: " name: new-app", Delta: difflib.RightOnly},

The dyff output format was always using dyff.DetectRenames(true), which overrode helm-diff's --find-renames option. This caused renames detected by helm-diff to be re-evaluated by dyff, resulting in inconsistent output compared to other formats (diff, simple). Fixes #632 Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

diff/report.go:124

Errors from dyff.CompareInputFiles and reportWriter.WriteReport are discarded. If dyff fails to compare or render (e.g., due to YAML parsing issues), the command can appear to succeed while producing incomplete output. Please handle these errors (at least surface them to stderr / the writer) so failures are diagnosable.

	report, _ := dyff.CompareInputFiles(currentInputFile, newInputFile, compareOptions...)
	reportWriter := &dyff.HumanReport{
		Report:               report,
		OmitHeader:           true,
		MinorChangeThreshold: 0.1,
	}
	_ = reportWriter.WriteReport(to)

Copilot · 2026-03-24T05:29:46Z

diff/report.go

 	currentInputFile, newInputFile, _ := ytbx.LoadFiles(currentFile.Name(), newFile.Name())

-	report, _ := dyff.CompareInputFiles(
-		currentInputFile, newInputFile,
+	var compareOptions []dyff.CompareOption
+	compareOptions = append(compareOptions,
 		dyff.IgnoreWhitespaceChanges(true),


The return error from ytbx.LoadFiles is currently ignored. If loading/parsing either temp file fails, dyff comparison may silently produce empty/incorrect output. Consider checking the error and writing a clear message (or falling back to the default diff output) instead of proceeding with a potentially invalid input state.

Copilot · 2026-03-24T05:29:46Z

diff/report_test.go

+func TestPrintDyffReportEmpty(t *testing.T) {
+	report := &Report{
+		Entries: []ReportEntry{},
+	}
+
+	var buf bytes.Buffer
+	printDyffReport(report, &buf)
+
+	output := buf.String()
+	require.Equal(t, "\n", output)
+}


The empty-report assertion is very strict (expects exactly a single newline). Since dyff's human output formatting can change across dyff versions, this may be brittle. Consider asserting that the output is empty/whitespace-only (or that it contains no diff markers) rather than matching an exact string.

yxxhero requested a review from Copilot March 24, 2026 01:46

Copilot started reviewing on behalf of yxxhero March 24, 2026 01:46 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

yxxhero force-pushed the fix-dyff-find-renames branch from 7dbe3f3 to b52aaab Compare March 24, 2026 03:40

yxxhero requested a review from Copilot March 24, 2026 03:49

Copilot started reviewing on behalf of yxxhero March 24, 2026 03:50 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

yxxhero force-pushed the fix-dyff-find-renames branch from b52aaab to 9d5217b Compare March 24, 2026 05:13

yxxhero requested a review from Copilot March 24, 2026 05:25

Copilot started reviewing on behalf of yxxhero March 24, 2026 05:26 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

yxxhero merged commit 1ce0182 into master Mar 24, 2026
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make --find-renames work with --output=dyff#961

fix: make --find-renames work with --output=dyff#961
yxxhero merged 1 commit intomasterfrom
fix-dyff-find-renames

yxxhero commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Copilot AI Mar 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		require.True(t, strings.Contains(output, "replicas") \|\| strings.Contains(output, "2") \|\| strings.Contains(output, "3"),
		"Expected dyff output to contain information about replicas change")

		{Payload: "- name: old-app", Delta: difflib.LeftOnly},
		{Payload: "+ name: new-app", Delta: difflib.RightOnly},

Conversation

yxxhero commented Mar 24, 2026

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants