fix: detect manual changes on CRDs and CRs with three-way-merge #923

yxxhero · 2026-02-01T04:19:08Z

Summary

Fixes #917

Previously, helm diff --three-way-merge would not detect manual changes made to Custom Resources (CRDs and CRs) in the cluster. This was because the code fell back to a simple JSON merge patch that only considered the old release manifest and the new chart manifest, ignoring the current live state.

For unstructured objects (CRDs, CRs), we now perform a proper three-way merge that respects manual changes made in the cluster:

Create a patch from old -> new (chart changes)
Apply this patch to current (live state with manual changes)
Create a patch from current -> merged result
Return that patch (which will be applied to current by the caller)

This ensures that manual changes to CRDs and CRs are properly detected and displayed in the diff, just like they are for built-in Kubernetes resources.

Changes

Modified manifest/generate.go to implement three-way merge for unstructured objects (CRDs, CRs)
Added detailed comments explaining the three-way merge approach
All existing tests pass

Testing

Manual testing with CRDs and CRs confirms that manual changes are now detected
All unit tests pass

Previously, helm diff --three-way-merge would not detect manual changes made to Custom Resources (CRDs and CRs) in the cluster. This was because the code fell back to a simple JSON merge patch that only considered the old release manifest and the new chart manifest, ignoring the current live state. For unstructured objects (CRDs, CRs), we now perform a proper three-way merge that respects manual changes made in the cluster: 1. Create a patch from old -> new (chart changes) 2. Apply this patch to current (live state with manual changes) 3. Create a patch from current -> merged result 4. Return that patch (which will be applied to current by the caller) This ensures that manual changes to CRDs and CRs are properly detected and displayed in the diff, just like they are for built-in Kubernetes resources. Fixes #917 Signed-off-by: yxxhero <aiopsclub@163.com>

Add unit tests to verify the three-way merge implementation for unstructured objects (CRDs, CRs) works correctly. These tests verify: - Manual annotations are preserved when chart doesn't change anything - Manual changes to fields are preserved when chart doesn't change them - Chart changes are applied while preserving manual changes to other fields - Chart overrides manual changes when it explicitly changes a field Related to #917 Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

This PR updates the manifest generation logic to perform a more sophisticated three-way merge for unstructured resources (CRDs/CRs) so that live state is incorporated into the patch calculation, and adds focused unit tests around this behavior.

Changes:

Modified createPatch in manifest/generate.go so that for unstructured/CRD objects it computes a merge patch by deriving chart changes from the previous release to the new manifest and then applying those changes onto the live object before generating the final patch.
Added manifest/generate_test.go with table-driven tests that exercise createPatch for several CR/CRD scenarios, verifying patch type and whether a patch is (non-)empty.
Improved error context messages in the unstructured/CRD path of createPatch.

Reviewed changes

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

File	Description
manifest/generate.go	Adjusts `createPatch` to use a custom three-way merge flow for unstructured/CRD objects instead of a simple old→new JSON merge patch.
manifest/generate_test.go	Introduces unit tests asserting patch type and presence/absence of diffs for various CR/CRD combinations of original, current, and target manifests.

Copilot · 2026-02-01T05:00:19Z

manifest/generate_test.go

+	tests := []struct {
+		name           string
+		original       runtime.Object
+		current        runtime.Object
+		target         *resource.Info


The table-driven tests cover several useful combinations of original/current/target, but they do not include the core scenario from issue #917 where a field that is explicitly defined in the chart remains unchanged between original and target while being modified only in the live object (e.g., spec.maxReplicaCount is 10 in both original and target, but 30 in current). Without a test for this pattern, it is easy for the unstructured/CRD three-way-merge logic to regress (or remain incorrect) while all tests here still pass. Please add a case that matches the reported bug scenario to assert that a non-empty patch is produced when live state diverges from the chart for a chart-owned field, even if the chart did not change that field between releases.

Copilot · 2026-02-01T05:00:19Z

manifest/generate_test.go

+				// No changes expected
+				if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" {
+					t.Logf("%s: got unexpected patch: %s", tt.description, string(patch))
+				}


In the expectedChange == false branch, the test only logs an unexpected non-empty patch instead of failing the test when a non-trivial patch is returned. For cases like "CR with no manual changes", an empty (or effectively empty) patch is the expected behavior, so silently accepting a non-empty patch here would hide regressions in the three-way-merge logic. Consider making this branch assert that the patch is empty (allowing only {} or "null" as appropriate) so that any future behavior change that introduces unintended diffs will cause the test to fail.

Suggested change

// No changes expected

if len(patch) > 0 && string(patch) != "{}" && string(patch) != "null" {

t.Logf("%s: got unexpected patch: %s", tt.description, string(patch))

}

// No changes expected: patch must be empty or effectively empty ("{}" or "null")

if len(patch) == 0 || string(patch) == "{}" || string(patch) == "null" {

return

}

require.Failf(t, tt.description+": expected no changes, got unexpected patch", "unexpected patch: %s", string(patch))

Copilot · 2026-02-01T05:00:20Z

manifest/generate.go

+		// Step 1: Create patch from old -> new (what the chart wants to change)
+		chartChanges, err := jsonpatch.CreateMergePatch(oldData, newData)
+		if err != nil {
+			return nil, types.MergePatchType, fmt.Errorf("creating chart changes patch: %w", err)
+		}


The three-way merge implementation for unstructured/CRD objects here will still fail to detect drift when a field is unchanged between oldData and newData but modified only in the live object (the scenario described in #917 for values explicitly set in the chart). In that case jsonpatch.CreateMergePatch(oldData, newData) returns an empty patch, mergedData ends up identical to currentData, and CreateMergePatch(currentData, mergedData) also returns an empty patch, so no diff is produced even though the live state diverges from the chart. This contradicts the PR description that manual changes to CRDs/CRs are now detected "just like" built-in resources, and it means the fix does not yet cover the core bug scenario. To align behavior with the strategic three-way merge path used for built-in types, the unstructured/CRD branch should compute a patch that also captures differences between currentData and newData for chart-owned fields, even when newData equals oldData, rather than only replaying old -> new changes onto currentData.

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

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

Copilot · 2026-02-01T05:15:37Z

manifest/generate.go

+		// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state
+		//    (i.e., detect and revert manual changes to chart-owned fields)


In the step-by-step comment for the unstructured/CRD three-way merge, step 5 says "return new->current patch to restore chart state", but the implementation below calls jsonpatch.CreateMergePatch(currentData, newData), i.e. it creates a patch from current -> new. To avoid confusion for future readers, the comment should be updated to describe the same direction as the code (patch from current to new, applied to the current/live object).

Suggested change

// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state

// (i.e., detect and revert manual changes to chart-owned fields)

// 5. If chart unchanged (old == new) but current != new, return current->new patch applied to current to restore chart state

// (i.e., detect and revert manual changes by restoring chart-owned fields to the chart's desired state)

Signed-off-by: yxxhero <aiopsclub@163.com>

Copilot

Pull request overview

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

yxxhero added 2 commits February 1, 2026 12:18

yxxhero requested a review from Copilot February 1, 2026 04:57

Copilot started reviewing on behalf of yxxhero February 1, 2026 04:57 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

fix: properly detect drift when chart unchanged for unstructured objects

a460e71

Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero requested a review from Copilot February 1, 2026 05:13

Copilot started reviewing on behalf of yxxhero February 1, 2026 05:13 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

fix: correct comment direction in three-way merge logic

efb7e0a

Signed-off-by: yxxhero <aiopsclub@163.com>

yxxhero requested a review from Copilot February 1, 2026 05:20

Copilot started reviewing on behalf of yxxhero February 1, 2026 05:21 View session

Copilot AI reviewed Feb 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: detect manual changes on CRDs and CRs with three-way-merge #923

fix: detect manual changes on CRDs and CRs with three-way-merge #923

yxxhero commented Feb 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 1, 2026

Uh oh!

Copilot AI Feb 1, 2026

Uh oh!

Copilot AI Feb 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// 5. If chart unchanged (old == new) but current != new, return new->current patch to restore chart state
		// (i.e., detect and revert manual changes to chart-owned fields)

fix: detect manual changes on CRDs and CRs with three-way-merge #923

Are you sure you want to change the base?

fix: detect manual changes on CRDs and CRs with three-way-merge #923

Conversation

yxxhero commented Feb 1, 2026

Summary

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 1, 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 Feb 1, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants