From 24aa83d37c171bd3eae02df56a85d76426cdc3bd Mon Sep 17 00:00:00 2001 From: Keerthan Reddy Mala Date: Thu, 6 Jul 2023 17:13:36 -0700 Subject: [PATCH 1/3] deep copy slice object during append --- merge/conflict.go | 2 +- merge/conflict_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/merge/conflict.go b/merge/conflict.go index 75a492d8..f1aa2586 100644 --- a/merge/conflict.go +++ b/merge/conflict.go @@ -112,7 +112,7 @@ func ConflictsFromManagers(sets fieldpath.ManagedFields) Conflicts { set.Set().Iterate(func(p fieldpath.Path) { conflicts = append(conflicts, Conflict{ Manager: manager, - Path: p, + Path: p.Copy(), }) }) } diff --git a/merge/conflict_test.go b/merge/conflict_test.go index f674f87c..685fde98 100644 --- a/merge/conflict_test.go +++ b/merge/conflict_test.go @@ -17,6 +17,7 @@ limitations under the License. package merge_test import ( + "reflect" "testing" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" @@ -92,3 +93,44 @@ func TestToSet(t *testing.T) { t.Fatalf("expected\n%v\n, but got\n%v\n", expected, actual) } } + +func TestConflictsFromManagers(t *testing.T) { + type args struct { + sets fieldpath.ManagedFields + } + tests := []struct { + name string + args args + want string + }{ + { + name: "test with common prefix", + args: args{ + sets: fieldpath.ManagedFields{ + "Bob": fieldpath.NewVersionedSet( + _NS( + _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "exec", "command"), + _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "periodSeconds"), + _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "exec", "command"), + _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "periodSeconds"), + ), + "v1", + false, + ), + }, + }, + want: `conflicts with "Bob": +- .spec.template.spec.containers[name="probe"].livenessProbe.periodSeconds +- .spec.template.spec.containers[name="probe"].livenessProbe.exec.command +- .spec.template.spec.containers[name="probe"].readinessProbe.periodSeconds +- .spec.template.spec.containers[name="probe"].readinessProbe.exec.command`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := merge.ConflictsFromManagers(tt.args.sets); !reflect.DeepEqual(got.Error(), tt.want) { + t.Errorf("ConflictsFromManagers() = %v, want %v", got, tt.want) + } + }) + } +} From 6371a4d18e3791106a90b1eaf8e771705d0ec44a Mon Sep 17 00:00:00 2001 From: Keerthan Reddy Mala Date: Fri, 7 Jul 2023 10:07:36 -0700 Subject: [PATCH 2/3] simplify test --- merge/conflict_test.go | 54 +++++++++++++----------------------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/merge/conflict_test.go b/merge/conflict_test.go index 685fde98..8d816948 100644 --- a/merge/conflict_test.go +++ b/merge/conflict_test.go @@ -17,7 +17,6 @@ limitations under the License. package merge_test import ( - "reflect" "testing" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" @@ -95,42 +94,21 @@ func TestToSet(t *testing.T) { } func TestConflictsFromManagers(t *testing.T) { - type args struct { - sets fieldpath.ManagedFields - } - tests := []struct { - name string - args args - want string - }{ - { - name: "test with common prefix", - args: args{ - sets: fieldpath.ManagedFields{ - "Bob": fieldpath.NewVersionedSet( - _NS( - _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "exec", "command"), - _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "livenessProbe", "periodSeconds"), - _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "exec", "command"), - _P("spec", "template", "spec", "containers", _KBF("name", "probe"), "readinessProbe", "periodSeconds"), - ), - "v1", - false, - ), - }, - }, - want: `conflicts with "Bob": -- .spec.template.spec.containers[name="probe"].livenessProbe.periodSeconds -- .spec.template.spec.containers[name="probe"].livenessProbe.exec.command -- .spec.template.spec.containers[name="probe"].readinessProbe.periodSeconds -- .spec.template.spec.containers[name="probe"].readinessProbe.exec.command`, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := merge.ConflictsFromManagers(tt.args.sets); !reflect.DeepEqual(got.Error(), tt.want) { - t.Errorf("ConflictsFromManagers() = %v, want %v", got, tt.want) - } - }) + + got := merge.ConflictsFromManagers(fieldpath.ManagedFields{ + "Bob": fieldpath.NewVersionedSet( + _NS( + _P("obj", "template", "obj", "list", _KBF("name", "a"), "id"), + _P("obj", "template", "obj", "list", _KBF("name", "a"), "key"), + ), + "v1", + false, + ), + }) + wanted := `conflicts with "Bob": +- .obj.template.obj.list[name="a"].id +- .obj.template.obj.list[name="a"].key` + if got.Error() != wanted { + t.Errorf("Got %v, wanted %v", got.Error(), wanted) } } From 69bc7e73e77e188b73c3b30dfd2ad62227d3de75 Mon Sep 17 00:00:00 2001 From: Keerthan Reddy Mala Date: Fri, 7 Jul 2023 11:09:31 -0700 Subject: [PATCH 3/3] address review comment --- merge/conflict_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/merge/conflict_test.go b/merge/conflict_test.go index 8d816948..49841856 100644 --- a/merge/conflict_test.go +++ b/merge/conflict_test.go @@ -94,7 +94,6 @@ func TestToSet(t *testing.T) { } func TestConflictsFromManagers(t *testing.T) { - got := merge.ConflictsFromManagers(fieldpath.ManagedFields{ "Bob": fieldpath.NewVersionedSet( _NS(