From d46c0e972a4ceaf289ed16e90f6c57a9579ba27f Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Sat, 2 May 2020 17:18:55 +0200 Subject: [PATCH 01/17] add ignoredFields to update --- internal/fixture/state.go | 101 +++++++++++++++++++++++---- merge/ignore_test.go | 120 ++++++++++++++++++++++++++++++++ merge/obsolete_versions_test.go | 6 +- merge/update.go | 8 +-- 4 files changed, 213 insertions(+), 22 deletions(-) create mode 100644 merge/ignore_test.go diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 7d93ee1e..a0550955 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -109,7 +109,7 @@ func (s *State) checkInit(version fieldpath.APIVersion) error { return nil } -func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, manager string) error { +func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string) error { err := s.checkInit(version) if err != nil { return err @@ -118,7 +118,7 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, manager) + newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, ignored, manager) if err != nil { return err } @@ -129,12 +129,12 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Update the current state with the passed in object -func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, manager string) error { +func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err } - return s.UpdateObject(tv, version, manager) + return s.UpdateObject(tv, version, ignored, manager) } func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, manager string, force bool) error { @@ -344,15 +344,16 @@ func (f ForceApplyObject) preprocess(parser Parser) (Operation, error) { // Update is a type of operation. It is a controller type of // update. Errors are passed along. type Update struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject + IgnoredFields *fieldpath.Set } var _ Operation = &Update{} func (u Update) run(state *State) error { - return state.Update(u.Object, u.APIVersion, u.Manager) + return state.Update(u.Object, u.APIVersion, u.IgnoredFields, u.Manager) } func (u Update) preprocess(parser Parser) (Operation, error) { @@ -361,24 +362,26 @@ func (u Update) preprocess(parser Parser) (Operation, error) { return nil, err } return UpdateObject{ - Manager: u.Manager, - APIVersion: u.APIVersion, - Object: tv, + Manager: u.Manager, + APIVersion: u.APIVersion, + Object: tv, + IgnoredFields: u.IgnoredFields, }, nil } // UpdateObject is a type of operation. It is a controller type of // update. Errors are passed along. type UpdateObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue + IgnoredFields *fieldpath.Set } var _ Operation = &Update{} func (u UpdateObject) run(state *State) error { - return state.UpdateObject(u.Object, u.APIVersion, u.Manager) + return state.UpdateObject(u.Object, u.APIVersion, u.IgnoredFields, u.Manager) } func (f UpdateObject) preprocess(parser Parser) (Operation, error) { @@ -505,3 +508,71 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e return nil } + +// PrintState is an Operation printing the current state to help with debugging tests +type PrintState struct{} + +var _ Operation = PrintState{} + +func (op PrintState) run(s *State) error { + fmt.Println(value.ToString(s.Live.AsValue())) + return nil +} + +func (op PrintState) preprocess(_ Parser) (Operation, error) { + return op, nil +} + +// ExpectState is an Operation comparing the current state to the defined config to help with debugging tests +type ExpectState struct { + APIVersion fieldpath.APIVersion + Object typed.YAMLObject +} + +var _ Operation = ExpectState{} + +func (op ExpectState) run(state *State) error { + comparison, err := state.CompareLive(op.Object, op.APIVersion) + if err != nil { + return fmt.Errorf("failed to compare live with config: %v", err) + } + if !comparison.IsSame() { + config, err := state.Parser.Type(string(op.APIVersion)).FromYAML(FixTabsOrDie(op.Object)) + if err != nil { + return fmt.Errorf("failed to parse config: %w", err) + } + return fmt.Errorf("expected live and config to be the same:\n%v\nConfig: %v\nLive: %v\n", comparison, value.ToString(config.AsValue()), value.ToString(state.Live.AsValue())) + } + return nil +} + +func (op ExpectState) preprocess(parser Parser) (Operation, error) { + return op, nil +} + +// ExpectManagedFields is an Operation checking if the manager owns the defined fields in the current state +// If the Fields are nil, it won't be an error if the manager is missing +type ExpectManagedFields struct { + Manager string + Fields *fieldpath.Set +} + +var _ Operation = ExpectManagedFields{} + +func (op ExpectManagedFields) run(state *State) error { + manager, ok := state.Managers[op.Manager] + if !ok { + if op.Fields == nil { + return nil + } + return fmt.Errorf("manager not found: %s", op.Manager) + } + if diff := manager.Set().Difference(op.Fields); !diff.Empty() { + return fmt.Errorf("unexpected managedFields for %s: \n%v", op.Manager, diff) + } + return nil +} + +func (op ExpectManagedFields) preprocess(parser Parser) (Operation, error) { + return op, nil +} diff --git a/merge/ignore_test.go b/merge/ignore_test.go new file mode 100644 index 00000000..1a93cf11 --- /dev/null +++ b/merge/ignore_test.go @@ -0,0 +1,120 @@ +package merge_test + +import ( + "testing" + + "sigs.k8s.io/structured-merge-diff/v3/fieldpath" + . "sigs.k8s.io/structured-merge-diff/v3/internal/fixture" +) + +func TestIgnoredFields(t *testing.T) { + tests := map[string]TestCase{ + "do_not_own_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + IgnoredFields: fieldpath.NewSet( + fieldpath.MakePathOrDie("string"), + ), + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + ), + }, + }, + }, + "do_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + fieldpath.MakePathOrDie("string"), + ), + }, + Update{ + Manager: "default2", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "no string" + `, + IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "no string" + `, + }, + ExpectManagedFields{ + Manager: "default2", + Fields: nil, + }, + }, + }, + "do_not_own_deep_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "default", + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + IgnoredFields: fieldpath.NewSet( + fieldpath.MakePathOrDie("obj"), + ), + }, + ExpectState{ + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + ), + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.Test(DeducedParser); err != nil { + t.Fatal("Should fail:", err) + } + }) + } +} diff --git a/merge/obsolete_versions_test.go b/merge/obsolete_versions_test.go index 751b63c3..9498a9c8 100644 --- a/merge/obsolete_versions_test.go +++ b/merge/obsolete_versions_test.go @@ -56,16 +56,16 @@ func TestObsoleteVersions(t *testing.T) { Parser: DeducedParser, } - if err := state.Update(typed.YAMLObject(`{"v1": 0}`), fieldpath.APIVersion("v1"), "v1"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0}`), fieldpath.APIVersion("v1"), nil, "v1"); err != nil { t.Fatalf("Failed to apply: %v", err) } - if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0}`), fieldpath.APIVersion("v2"), "v2"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0}`), fieldpath.APIVersion("v2"), nil, "v2"); err != nil { t.Fatalf("Failed to apply: %v", err) } // Remove v1, add v3 instead. converter.AcceptedVersions = []fieldpath.APIVersion{"v2", "v3"} - if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0, "v3": 0}`), fieldpath.APIVersion("v3"), "v3"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0, "v3": 0}`), fieldpath.APIVersion("v3"), nil, "v3"); err != nil { t.Fatalf("Failed to apply: %v", err) } diff --git a/merge/update.go b/merge/update.go index 3c2a4690..e4815e5b 100644 --- a/merge/update.go +++ b/merge/update.go @@ -41,7 +41,7 @@ func (s *Updater) EnableUnionFeature() { s.enableUnions = true } -func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, workflow string, force bool) (fieldpath.ManagedFields, *typed.Comparison, error) { +func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, workflow string, force bool) (fieldpath.ManagedFields, *typed.Comparison, error) { conflicts := fieldpath.ManagedFields{} removed := fieldpath.ManagedFields{} compare, err := oldObject.Compare(newObject) @@ -119,7 +119,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa // that you intend to persist (after applying the patch if this is for a // PATCH call), and liveObject must be the original object (empty if // this is a CREATE call). -func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) { +func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) { var err error if s.enableUnions { newObject, err = liveObject.NormalizeUnions(newObject) @@ -128,7 +128,7 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp } } managers = shallowCopyManagers(managers) - managers, compare, err := s.update(liveObject, newObject, version, managers, manager, true) + managers, compare, err := s.update(liveObject, newObject, version, managers, ignored, manager, true) if err != nil { return nil, fieldpath.ManagedFields{}, err } @@ -179,7 +179,7 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to prune fields: %v", err) } - managers, compare, err := s.update(liveObject, newObject, version, managers, manager, force) + managers, compare, err := s.update(liveObject, newObject, version, managers, nil, manager, force) if err != nil { return nil, fieldpath.ManagedFields{}, err } From 7c8a53d47bdf3005609932b96a0d8cfafe249a0d Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 11 May 2020 20:08:14 +0200 Subject: [PATCH 02/17] implement set.RecursiveDifference --- fieldpath/set.go | 47 ++++++++++++ typed/comparison_test.go | 161 +++++++++++++++++++++++++++++++++++++++ typed/typed.go | 10 +++ 3 files changed, 218 insertions(+) create mode 100644 typed/comparison_test.go diff --git a/fieldpath/set.go b/fieldpath/set.go index f280a2fc..5afc36ce 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -94,6 +94,53 @@ func (s *Set) Difference(s2 *Set) *Set { } } +// RecursiveDifference returns a Set containing elements which: +// * appear in s +// * do not appear in s2 +// +// Compared to a regular difference, this recursively removes +// all children from s +// that are either children or members in s2 +func (s *Set) RecursiveDifference(s2 *Set) *Set { + out := &Set{ + Members: *s.Members.Difference(&s2.Members), + } + + i, j := 0, 0 + for i < len(s.Children.members) && j < len(s2.Children.members) { + if s.Children.members[i].pathElement.Less(s2.Children.members[j].pathElement) { + if !s2.Members.Has(s.Children.members[i].pathElement) { + out.Children.members = append(out.Children.members, setNode{ + pathElement: s.Children.members[i].pathElement, + set: s.Children.members[i].set, + }) + } + i++ + } else { + if !s2.Children.members[j].pathElement.Less(s.Children.members[i].pathElement) { + if !s2.Members.Has(s.Children.members[i].pathElement) { + diff := s.Children.members[i].set.RecursiveDifference(s2.Children.members[j].set) + if !diff.Empty() { + out.Children.members = append(out.Children.members, setNode{pathElement: s.Children.members[i].pathElement, set: diff}) + } + } + i++ + } + j++ + } + } + + if i < len(s.Children.members) { + for _, c := range s.Children.members[i:] { + if !s2.Members.Has(c.pathElement) { + out.Children.members = append(out.Children.members, c) + } + } + } + + return out +} + // Size returns the number of members of the set. func (s *Set) Size() int { return s.Members.Size() + s.Children.Size() diff --git a/typed/comparison_test.go b/typed/comparison_test.go new file mode 100644 index 00000000..ca246da3 --- /dev/null +++ b/typed/comparison_test.go @@ -0,0 +1,161 @@ +package typed_test + +import ( + "testing" + + "sigs.k8s.io/structured-merge-diff/v3/fieldpath" + "sigs.k8s.io/structured-merge-diff/v3/typed" +) + +func TestComparisonRemove(t *testing.T) { + cases := []struct { + name string + Comparison *typed.Comparison + Remove *fieldpath.Set + Expect *typed.Comparison + Fails bool + }{ + { + name: "works on nil set", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + Remove: nil, + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + }, + { + name: "works on empty set", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + Remove: fieldpath.NewSet(), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + }, + { + name: "does not result in empty comparison on empty set", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), + }, + Remove: fieldpath.NewSet(), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + Fails: true, + }, + { + name: "removes simple nested paths", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + fieldpath.MakePathOrDie("a", "ab", "aba"), + ), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + Remove: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + ), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + }, + { + name: "removes nested paths", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + fieldpath.MakePathOrDie("a", "aa", "aaa"), + fieldpath.MakePathOrDie("a", "aa", "aab"), + fieldpath.MakePathOrDie("a", "ab", "aba"), + fieldpath.MakePathOrDie("a", "ab", "abb"), + ), + Modified: fieldpath.NewSet( + fieldpath.MakePathOrDie("b"), + fieldpath.MakePathOrDie("b", "ba", "baa"), + fieldpath.MakePathOrDie("b", "ba", "bab"), + fieldpath.MakePathOrDie("b", "bb", "bba"), + fieldpath.MakePathOrDie("b", "bb", "bbb"), + ), + Removed: fieldpath.NewSet( + fieldpath.MakePathOrDie("c"), + fieldpath.MakePathOrDie("c", "ca", "caa"), + fieldpath.MakePathOrDie("c", "ca", "cab"), + fieldpath.MakePathOrDie("c", "cb", "cba"), + fieldpath.MakePathOrDie("c", "cb", "cbb"), + ), + }, + Remove: fieldpath.NewSet( + fieldpath.MakePathOrDie("a", "aa"), + fieldpath.MakePathOrDie("b", "bb", "bba"), + fieldpath.MakePathOrDie("c"), + ), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + fieldpath.MakePathOrDie("a", "ab", "aba"), + fieldpath.MakePathOrDie("a", "ab", "abb"), + ), + Modified: fieldpath.NewSet( + fieldpath.MakePathOrDie("b"), + fieldpath.MakePathOrDie("b", "ba", "baa"), + fieldpath.MakePathOrDie("b", "ba", "bab"), + fieldpath.MakePathOrDie("b", "bb", "bbb"), + ), + Removed: fieldpath.NewSet(), + }, + }, + { + name: "does not remove every child", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a", "aa", "aaa"), + fieldpath.MakePathOrDie("a", "aa", "aab"), + fieldpath.MakePathOrDie("a", "ab", "aba"), + fieldpath.MakePathOrDie("a", "ab", "abb"), + ), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + Remove: fieldpath.NewSet( + fieldpath.MakePathOrDie("a", "ab"), + ), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a", "aa", "aaa"), + fieldpath.MakePathOrDie("a", "aa", "aab"), + ), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + c.Comparison.Remove(c.Remove) + if (!c.Comparison.Added.Equals(c.Expect.Added) || + !c.Comparison.Modified.Equals(c.Expect.Modified) || + !c.Comparison.Removed.Equals(c.Expect.Removed)) != c.Fails { + t.Fatalf("remove expected: \n%v\nremoved:\n%v\ngot:\n%v\n", c.Expect, c.Remove, c.Comparison) + } + }) + } +} diff --git a/typed/typed.go b/typed/typed.go index 4aa9a238..77b20e35 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -291,3 +291,13 @@ func (c *Comparison) String() string { } return bld.String() } + +// Remove fields from the compare +func (c *Comparison) Remove(fields *fieldpath.Set) { + if fields == nil || fields.Empty() { + return + } + c.Removed = c.Removed.RecursiveDifference(fields) + c.Modified = c.Modified.RecursiveDifference(fields) + c.Added = c.Added.RecursiveDifference(fields) +} From c54f8115ec02851dd5c372d6defe52bd2facabbe Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 11 May 2020 20:29:10 +0200 Subject: [PATCH 03/17] add ignored fields to apply --- internal/fixture/state.go | 65 ++++++++++-------- merge/ignore_test.go | 114 +++++++++++++++++++++++++++++++- merge/obsolete_versions_test.go | 4 +- merge/update.go | 8 ++- 4 files changed, 156 insertions(+), 35 deletions(-) diff --git a/internal/fixture/state.go b/internal/fixture/state.go index a0550955..05caa9c9 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -137,7 +137,7 @@ func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignor return s.UpdateObject(tv, version, ignored, manager) } -func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, manager string, force bool) error { +func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string, force bool) error { err := s.checkInit(version) if err != nil { return err @@ -146,7 +146,7 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - new, managers, err := s.Updater.Apply(s.Live, tv, version, s.Managers, manager, force) + new, managers, err := s.Updater.Apply(s.Live, tv, version, s.Managers, ignored, manager, force) if err != nil { return err } @@ -158,12 +158,12 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Apply the passed in object to the current state -func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, manager string, force bool) error { +func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string, force bool) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err } - return s.ApplyObject(tv, version, manager, force) + return s.ApplyObject(tv, version, ignored, manager, force) } // CompareLive takes a YAML string and returns the comparison with the @@ -231,10 +231,11 @@ func addedConflicts(one, other merge.Conflicts) merge.Conflicts { // conflict, the user can specify the expected conflicts. If conflicts // don't match, an error will occur. type Apply struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject - Conflicts merge.Conflicts + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject + Conflicts merge.Conflicts + IgnoredFields *fieldpath.Set } var _ Operation = &Apply{} @@ -253,24 +254,26 @@ func (a Apply) preprocess(parser Parser) (Operation, error) { return nil, err } return ApplyObject{ - Manager: a.Manager, - APIVersion: a.APIVersion, - Object: tv, - Conflicts: a.Conflicts, + Manager: a.Manager, + APIVersion: a.APIVersion, + Object: tv, + Conflicts: a.Conflicts, + IgnoredFields: a.IgnoredFields, }, nil } type ApplyObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue - Conflicts merge.Conflicts + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue + Conflicts merge.Conflicts + IgnoredFields *fieldpath.Set } var _ Operation = &ApplyObject{} func (a ApplyObject) run(state *State) error { - err := state.ApplyObject(a.Object, a.APIVersion, a.Manager, false) + err := state.ApplyObject(a.Object, a.APIVersion, a.IgnoredFields, a.Manager, false) if err != nil { if _, ok := err.(merge.Conflicts); !ok || a.Conflicts == nil { return err @@ -300,15 +303,16 @@ func (a ApplyObject) preprocess(parser Parser) (Operation, error) { // ForceApply is a type of operation. It is a forced-apply run by a // manager with a given object. Any error will be returned. type ForceApply struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject + IgnoredFields *fieldpath.Set } var _ Operation = &ForceApply{} func (f ForceApply) run(state *State) error { - return state.Apply(f.Object, f.APIVersion, f.Manager, true) + return state.Apply(f.Object, f.APIVersion, f.IgnoredFields, f.Manager, true) } func (f ForceApply) preprocess(parser Parser) (Operation, error) { @@ -317,24 +321,26 @@ func (f ForceApply) preprocess(parser Parser) (Operation, error) { return nil, err } return ForceApplyObject{ - Manager: f.Manager, - APIVersion: f.APIVersion, - Object: tv, + Manager: f.Manager, + APIVersion: f.APIVersion, + Object: tv, + IgnoredFields: f.IgnoredFields, }, nil } // ForceApplyObject is a type of operation. It is a forced-apply run by // a manager with a given object. Any error will be returned. type ForceApplyObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue + IgnoredFields *fieldpath.Set } var _ Operation = &ForceApplyObject{} func (f ForceApplyObject) run(state *State) error { - return state.ApplyObject(f.Object, f.APIVersion, f.Manager, true) + return state.ApplyObject(f.Object, f.APIVersion, f.IgnoredFields, f.Manager, true) } func (f ForceApplyObject) preprocess(parser Parser) (Operation, error) { @@ -567,6 +573,9 @@ func (op ExpectManagedFields) run(state *State) error { } return fmt.Errorf("manager not found: %s", op.Manager) } + if op.Fields == nil { + op.Fields = fieldpath.NewSet() + } if diff := manager.Set().Difference(op.Fields); !diff.Empty() { return fmt.Errorf("unexpected managedFields for %s: \n%v", op.Manager, diff) } diff --git a/merge/ignore_test.go b/merge/ignore_test.go index 1a93cf11..63401ac0 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -9,7 +9,7 @@ import ( func TestIgnoredFields(t *testing.T) { tests := map[string]TestCase{ - "do_not_own_ignored": { + "update_does_not_own_ignored": { APIVersion: "v1", Ops: []Operation{ Update{ @@ -38,7 +38,7 @@ func TestIgnoredFields(t *testing.T) { }, }, }, - "do_not_steal_ignored": { + "update_does_not_steal_ignored": { APIVersion: "v1", Ops: []Operation{ Update{ @@ -85,7 +85,7 @@ func TestIgnoredFields(t *testing.T) { }, }, }, - "do_not_own_deep_ignored": { + "update_does_not_own_deep_ignored": { APIVersion: "v1", Ops: []Operation{ Update{ @@ -108,6 +108,114 @@ func TestIgnoredFields(t *testing.T) { }, }, }, + "apply_does_not_own_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + IgnoredFields: fieldpath.NewSet( + fieldpath.MakePathOrDie("string"), + ), + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + ), + }, + }, + }, + "apply_does_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + fieldpath.MakePathOrDie("string"), + ), + }, + Apply{ + Manager: "default2", + APIVersion: "v1", + Object: ` + numeric: 1 + string: "no string" + `, + IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), + }, + ExpectState{ + APIVersion: "v1", + Object: ` + numeric: 1 + string: "some string" + `, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + fieldpath.MakePathOrDie("string"), + ), + }, + ExpectManagedFields{ + Manager: "default2", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + ), + }, + }, + }, + "apply_does_not_own_deep_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "default", + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + IgnoredFields: fieldpath.NewSet( + fieldpath.MakePathOrDie("obj"), + ), + }, + ExpectState{ + APIVersion: "v1", + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + }, + ExpectManagedFields{ + Manager: "default", + Fields: fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + ), + }, + }, + }, } for name, test := range tests { diff --git a/merge/obsolete_versions_test.go b/merge/obsolete_versions_test.go index 9498a9c8..4ac2413b 100644 --- a/merge/obsolete_versions_test.go +++ b/merge/obsolete_versions_test.go @@ -113,13 +113,13 @@ func TestApplyObsoleteVersion(t *testing.T) { Parser: SameVersionParser{T: parser.Type("sets")}, } - if err := state.Apply(typed.YAMLObject(`{"list": ["a", "b", "c", "d"]}`), fieldpath.APIVersion("v1"), "apply", false); err != nil { + if err := state.Apply(typed.YAMLObject(`{"list": ["a", "b", "c", "d"]}`), fieldpath.APIVersion("v1"), nil, "apply", false); err != nil { t.Fatalf("Failed to apply: %v", err) } // Remove v1, add v2 instead. converter.AcceptedVersions = []fieldpath.APIVersion{"v2"} - if err := state.Apply(typed.YAMLObject(`{"list": ["a"]}`), fieldpath.APIVersion("v2"), "apply", false); err != nil { + if err := state.Apply(typed.YAMLObject(`{"list": ["a"]}`), fieldpath.APIVersion("v2"), nil, "apply", false); err != nil { t.Fatalf("Failed to apply: %v", err) } diff --git a/merge/update.go b/merge/update.go index e4815e5b..4bd65a71 100644 --- a/merge/update.go +++ b/merge/update.go @@ -80,6 +80,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa if err != nil { return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } + compare.Remove(ignored) versions[managerSet.APIVersion()] = compare } @@ -150,7 +151,7 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp // well as the configuration that is applied. This will merge the object // and return it. If the object hasn't changed, nil is returned (the // managers can still have changed though). -func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) { +func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) { managers = shallowCopyManagers(managers) var err error if s.enableUnions { @@ -171,6 +172,9 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel } lastSet := managers[manager] set, err := configObject.ToFieldSet() + if ignored != nil { + set = set.RecursiveDifference(ignored) + } if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err) } @@ -179,7 +183,7 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to prune fields: %v", err) } - managers, compare, err := s.update(liveObject, newObject, version, managers, nil, manager, force) + managers, compare, err := s.update(liveObject, newObject, version, managers, ignored, manager, force) if err != nil { return nil, fieldpath.ManagedFields{}, err } From fd0df01982c4ef571c5d74a15d61eee85662d67c Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 11 May 2020 20:30:19 +0200 Subject: [PATCH 04/17] update vendors --- go.mod | 1 + go.sum | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index fae371bf..29e44aeb 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/json-iterator/go v1.1.6 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.1 // indirect + github.com/stretchr/testify v1.3.0 // indirect ) go 1.13 diff --git a/go.sum b/go.sum index e0f34a25..bde0aebc 100644 --- a/go.sum +++ b/go.sum @@ -17,5 +17,4 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.2.1 h1:mUhvW9EsL+naU5Q3cakzfE91YhliOondGd6ZrsDBHQE= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -sigs.k8s.io/structured-merge-diff v1.0.1 h1:LOs1LZWMsz1xs77Phr/pkB4LFaavH7IVq/3+WTN9XTA= -sigs.k8s.io/structured-merge-diff v1.0.1/go.mod h1:IIgPezJWb76P0hotTxzDbWsMYB8APh18qZnxkomBpxA= +sigs.k8s.io/structured-merge-diff v1.0.2 h1:WiMoyniAVAYm03w+ImfF9IE2G23GLR/SwDnQyaNZvPk= From eccbede94065f459a38caab2f8cfd3db31ccdaed Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 11 May 2020 21:40:52 +0200 Subject: [PATCH 05/17] fix update and apply --- merge/update.go | 18 +++++++++++++----- typed/comparison_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/merge/update.go b/merge/update.go index 4bd65a71..360ef6df 100644 --- a/merge/update.go +++ b/merge/update.go @@ -80,10 +80,11 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa if err != nil { return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } - compare.Remove(ignored) versions[managerSet.APIVersion()] = compare } + compare.Remove(ignored) + conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added)) if !conflictSet.Empty() { conflicts[manager] = fieldpath.NewVersionedSet(conflictSet, managerSet.APIVersion(), false) @@ -136,8 +137,11 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp if _, ok := managers[manager]; !ok { managers[manager] = fieldpath.NewVersionedSet(fieldpath.NewSet(), version, false) } + if ignored == nil { + ignored = fieldpath.NewSet() + } managers[manager] = fieldpath.NewVersionedSet( - managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed), + managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed).RecursiveDifference(ignored), version, false, ) @@ -172,12 +176,16 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel } lastSet := managers[manager] set, err := configObject.ToFieldSet() - if ignored != nil { - set = set.RecursiveDifference(ignored) - } if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err) } + if ignored != nil { + set = set.RecursiveDifference(ignored) + // TODO: is this correct. If we don't remove from lastSet pruning might remove the fields? + if lastSet != nil { + lastSet.Set().RecursiveDifference(ignored) + } + } managers[manager] = fieldpath.NewVersionedSet(set, version, true) newObject, err = s.prune(newObject, managers, manager, lastSet) if err != nil { diff --git a/typed/comparison_test.go b/typed/comparison_test.go index ca246da3..5387c85a 100644 --- a/typed/comparison_test.go +++ b/typed/comparison_test.go @@ -146,6 +146,30 @@ func TestComparisonRemove(t *testing.T) { Removed: fieldpath.NewSet(), }, }, + { + name: "removes simple path", + Comparison: &typed.Comparison{ + Added: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + ), + Modified: fieldpath.NewSet( + fieldpath.MakePathOrDie("b"), + ), + Removed: fieldpath.NewSet( + fieldpath.MakePathOrDie("c"), + ), + }, + Remove: fieldpath.NewSet( + fieldpath.MakePathOrDie("a"), + fieldpath.MakePathOrDie("b"), + fieldpath.MakePathOrDie("c"), + ), + Expect: &typed.Comparison{ + Added: fieldpath.NewSet(), + Modified: fieldpath.NewSet(), + Removed: fieldpath.NewSet(), + }, + }, } for _, c := range cases { From 4b4ef1265cde368c5b77e28a6f8008df0aa33b0e Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 11 May 2020 21:52:01 +0200 Subject: [PATCH 06/17] remove unused test code --- internal/fixture/state.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 05caa9c9..109babc7 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -515,20 +515,6 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e return nil } -// PrintState is an Operation printing the current state to help with debugging tests -type PrintState struct{} - -var _ Operation = PrintState{} - -func (op PrintState) run(s *State) error { - fmt.Println(value.ToString(s.Live.AsValue())) - return nil -} - -func (op PrintState) preprocess(_ Parser) (Operation, error) { - return op, nil -} - // ExpectState is an Operation comparing the current state to the defined config to help with debugging tests type ExpectState struct { APIVersion fieldpath.APIVersion From 85efcfffa57bd91654c7245f52788ad94afe2189 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 10:37:49 +0200 Subject: [PATCH 07/17] rename Remove to ExcludeFields --- merge/update.go | 2 +- typed/comparison_test.go | 4 ++-- typed/typed.go | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/merge/update.go b/merge/update.go index 360ef6df..f3270081 100644 --- a/merge/update.go +++ b/merge/update.go @@ -83,7 +83,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa versions[managerSet.APIVersion()] = compare } - compare.Remove(ignored) + compare.ExcludeFields(ignored) conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added)) if !conflictSet.Empty() { diff --git a/typed/comparison_test.go b/typed/comparison_test.go index 5387c85a..b21064e2 100644 --- a/typed/comparison_test.go +++ b/typed/comparison_test.go @@ -7,7 +7,7 @@ import ( "sigs.k8s.io/structured-merge-diff/v3/typed" ) -func TestComparisonRemove(t *testing.T) { +func TestComparisonExcludeFields(t *testing.T) { cases := []struct { name string Comparison *typed.Comparison @@ -174,7 +174,7 @@ func TestComparisonRemove(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - c.Comparison.Remove(c.Remove) + c.Comparison.ExcludeFields(c.Remove) if (!c.Comparison.Added.Equals(c.Expect.Added) || !c.Comparison.Modified.Equals(c.Expect.Modified) || !c.Comparison.Removed.Equals(c.Expect.Removed)) != c.Fails { diff --git a/typed/typed.go b/typed/typed.go index 77b20e35..09947cae 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -292,8 +292,9 @@ func (c *Comparison) String() string { return bld.String() } -// Remove fields from the compare -func (c *Comparison) Remove(fields *fieldpath.Set) { +// ExcludeFields fields from the compare recursively removes the fields +// from the entire comparison +func (c *Comparison) ExcludeFields(fields *fieldpath.Set) { if fields == nil || fields.Empty() { return } From 8a3560e95c3217f9d5ee92866bcb8e18730b8ad8 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 12:20:01 +0200 Subject: [PATCH 08/17] move compare test cases to set_test.go --- fieldpath/set_test.go | 56 +++++++++++++++++++ typed/comparison_test.go | 117 ++------------------------------------- 2 files changed, 60 insertions(+), 113 deletions(-) diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index fb06dd05..73f046ea 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -447,6 +447,62 @@ func TestSetIntersectionDifference(t *testing.T) { }) } +func TestSetRecursiveDifference(t *testing.T) { + table := []struct { + name string + a *Set + b *Set + expect *Set + }{ + { + name: "removes simple path", + a: NewSet(MakePathOrDie("a")), + b: NewSet(MakePathOrDie("a")), + expect: NewSet(), + }, + { + name: "removes direct path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a", "b", "c")), + expect: NewSet(), + }, + { + name: "only removes matching child", + a: NewSet( + MakePathOrDie("a", "b", "c"), + MakePathOrDie("b", "b", "c"), + ), + b: NewSet(MakePathOrDie("a", "b", "c")), + expect: NewSet(MakePathOrDie("b", "b", "c")), + }, + { + name: "removes nested path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a")), + expect: NewSet(), + }, + { + name: "only removes nested path for matching children", + a: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + b: NewSet(MakePathOrDie("a", "aa")), + expect: NewSet( + MakePathOrDie("a", "ab", "aba"), + ), + }, + } + + for _, c := range table { + t.Run(c.name, func(t *testing.T) { + if result := c.a.RecursiveDifference(c.b); !result.Equals(c.expect) { + t.Fatalf("expected: \n%v\n, got: \n%v\n", c.expect, result) + } + }) + } +} + func TestSetNodeMapIterate(t *testing.T) { set := &SetNodeMap{} toAdd := 5 diff --git a/typed/comparison_test.go b/typed/comparison_test.go index b21064e2..ef2934d6 100644 --- a/typed/comparison_test.go +++ b/typed/comparison_test.go @@ -44,120 +44,11 @@ func TestComparisonExcludeFields(t *testing.T) { }, }, { - name: "does not result in empty comparison on empty set", + name: "removes from entire object", Comparison: &typed.Comparison{ - Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a")), - Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b")), - Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c")), - }, - Remove: fieldpath.NewSet(), - Expect: &typed.Comparison{ - Added: fieldpath.NewSet(), - Modified: fieldpath.NewSet(), - Removed: fieldpath.NewSet(), - }, - Fails: true, - }, - { - name: "removes simple nested paths", - Comparison: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a"), - fieldpath.MakePathOrDie("a", "ab", "aba"), - ), - Modified: fieldpath.NewSet(), - Removed: fieldpath.NewSet(), - }, - Remove: fieldpath.NewSet( - fieldpath.MakePathOrDie("a"), - ), - Expect: &typed.Comparison{ - Added: fieldpath.NewSet(), - Modified: fieldpath.NewSet(), - Removed: fieldpath.NewSet(), - }, - }, - { - name: "removes nested paths", - Comparison: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a"), - fieldpath.MakePathOrDie("a", "aa", "aaa"), - fieldpath.MakePathOrDie("a", "aa", "aab"), - fieldpath.MakePathOrDie("a", "ab", "aba"), - fieldpath.MakePathOrDie("a", "ab", "abb"), - ), - Modified: fieldpath.NewSet( - fieldpath.MakePathOrDie("b"), - fieldpath.MakePathOrDie("b", "ba", "baa"), - fieldpath.MakePathOrDie("b", "ba", "bab"), - fieldpath.MakePathOrDie("b", "bb", "bba"), - fieldpath.MakePathOrDie("b", "bb", "bbb"), - ), - Removed: fieldpath.NewSet( - fieldpath.MakePathOrDie("c"), - fieldpath.MakePathOrDie("c", "ca", "caa"), - fieldpath.MakePathOrDie("c", "ca", "cab"), - fieldpath.MakePathOrDie("c", "cb", "cba"), - fieldpath.MakePathOrDie("c", "cb", "cbb"), - ), - }, - Remove: fieldpath.NewSet( - fieldpath.MakePathOrDie("a", "aa"), - fieldpath.MakePathOrDie("b", "bb", "bba"), - fieldpath.MakePathOrDie("c"), - ), - Expect: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a"), - fieldpath.MakePathOrDie("a", "ab", "aba"), - fieldpath.MakePathOrDie("a", "ab", "abb"), - ), - Modified: fieldpath.NewSet( - fieldpath.MakePathOrDie("b"), - fieldpath.MakePathOrDie("b", "ba", "baa"), - fieldpath.MakePathOrDie("b", "ba", "bab"), - fieldpath.MakePathOrDie("b", "bb", "bbb"), - ), - Removed: fieldpath.NewSet(), - }, - }, - { - name: "does not remove every child", - Comparison: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a", "aa", "aaa"), - fieldpath.MakePathOrDie("a", "aa", "aab"), - fieldpath.MakePathOrDie("a", "ab", "aba"), - fieldpath.MakePathOrDie("a", "ab", "abb"), - ), - Modified: fieldpath.NewSet(), - Removed: fieldpath.NewSet(), - }, - Remove: fieldpath.NewSet( - fieldpath.MakePathOrDie("a", "ab"), - ), - Expect: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a", "aa", "aaa"), - fieldpath.MakePathOrDie("a", "aa", "aab"), - ), - Modified: fieldpath.NewSet(), - Removed: fieldpath.NewSet(), - }, - }, - { - name: "removes simple path", - Comparison: &typed.Comparison{ - Added: fieldpath.NewSet( - fieldpath.MakePathOrDie("a"), - ), - Modified: fieldpath.NewSet( - fieldpath.MakePathOrDie("b"), - ), - Removed: fieldpath.NewSet( - fieldpath.MakePathOrDie("c"), - ), + Added: fieldpath.NewSet(fieldpath.MakePathOrDie("a", "aa")), + Modified: fieldpath.NewSet(fieldpath.MakePathOrDie("b", "ba")), + Removed: fieldpath.NewSet(fieldpath.MakePathOrDie("c", "ca")), }, Remove: fieldpath.NewSet( fieldpath.MakePathOrDie("a"), From 9810220206218c514a634b5c60d8e7eb7766ac12 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 12:37:20 +0200 Subject: [PATCH 09/17] cleanup tests --- internal/fixture/state.go | 57 -------------- merge/ignore_test.go | 160 ++++++++++++++++---------------------- 2 files changed, 68 insertions(+), 149 deletions(-) diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 109babc7..07ae1f5d 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -514,60 +514,3 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e return nil } - -// ExpectState is an Operation comparing the current state to the defined config to help with debugging tests -type ExpectState struct { - APIVersion fieldpath.APIVersion - Object typed.YAMLObject -} - -var _ Operation = ExpectState{} - -func (op ExpectState) run(state *State) error { - comparison, err := state.CompareLive(op.Object, op.APIVersion) - if err != nil { - return fmt.Errorf("failed to compare live with config: %v", err) - } - if !comparison.IsSame() { - config, err := state.Parser.Type(string(op.APIVersion)).FromYAML(FixTabsOrDie(op.Object)) - if err != nil { - return fmt.Errorf("failed to parse config: %w", err) - } - return fmt.Errorf("expected live and config to be the same:\n%v\nConfig: %v\nLive: %v\n", comparison, value.ToString(config.AsValue()), value.ToString(state.Live.AsValue())) - } - return nil -} - -func (op ExpectState) preprocess(parser Parser) (Operation, error) { - return op, nil -} - -// ExpectManagedFields is an Operation checking if the manager owns the defined fields in the current state -// If the Fields are nil, it won't be an error if the manager is missing -type ExpectManagedFields struct { - Manager string - Fields *fieldpath.Set -} - -var _ Operation = ExpectManagedFields{} - -func (op ExpectManagedFields) run(state *State) error { - manager, ok := state.Managers[op.Manager] - if !ok { - if op.Fields == nil { - return nil - } - return fmt.Errorf("manager not found: %s", op.Manager) - } - if op.Fields == nil { - op.Fields = fieldpath.NewSet() - } - if diff := manager.Set().Difference(op.Fields); !diff.Empty() { - return fmt.Errorf("unexpected managedFields for %s: \n%v", op.Manager, diff) - } - return nil -} - -func (op ExpectManagedFields) preprocess(parser Parser) (Operation, error) { - return op, nil -} diff --git a/merge/ignore_test.go b/merge/ignore_test.go index 63401ac0..c8263e80 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -23,19 +23,19 @@ func TestIgnoredFields(t *testing.T) { fieldpath.MakePathOrDie("string"), ), }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( + }, + Object: ` + numeric: 1 + string: "some string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), ), - }, + "v1", + false, + ), }, }, "update_does_not_steal_ignored": { @@ -49,20 +49,6 @@ func TestIgnoredFields(t *testing.T) { string: "some string" `, }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), - fieldpath.MakePathOrDie("string"), - ), - }, Update{ Manager: "default2", APIVersion: "v1", @@ -72,17 +58,20 @@ func TestIgnoredFields(t *testing.T) { `, IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "no string" - `, - }, - ExpectManagedFields{ - Manager: "default2", - Fields: nil, - }, + }, + Object: ` + numeric: 1 + string: "no string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( + fieldpath.MakePathOrDie("numeric"), + fieldpath.MakePathOrDie("string"), + ), + "v1", + false, + ), }, }, "update_does_not_own_deep_ignored": { @@ -96,16 +85,16 @@ func TestIgnoredFields(t *testing.T) { fieldpath.MakePathOrDie("obj"), ), }, - ExpectState{ - APIVersion: "v1", - Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( + }, + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), ), - }, + "v1", + false, + ), }, }, "apply_does_not_own_ignored": { @@ -122,19 +111,19 @@ func TestIgnoredFields(t *testing.T) { fieldpath.MakePathOrDie("string"), ), }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( + }, + Object: ` + numeric: 1 + string: "some string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), ), - }, + "v1", + true, + ), }, }, "apply_does_not_steal_ignored": { @@ -148,20 +137,6 @@ func TestIgnoredFields(t *testing.T) { string: "some string" `, }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), - fieldpath.MakePathOrDie("string"), - ), - }, Apply{ Manager: "default2", APIVersion: "v1", @@ -171,26 +146,27 @@ func TestIgnoredFields(t *testing.T) { `, IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), }, - ExpectState{ - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( + }, + Object: ` + numeric: 1 + string: "some string" + `, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), fieldpath.MakePathOrDie("string"), ), - }, - ExpectManagedFields{ - Manager: "default2", - Fields: fieldpath.NewSet( + "v1", + true, + ), + "default2": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), ), - }, + "v1", + true, + ), }, }, "apply_does_not_own_deep_ignored": { @@ -204,16 +180,16 @@ func TestIgnoredFields(t *testing.T) { fieldpath.MakePathOrDie("obj"), ), }, - ExpectState{ - APIVersion: "v1", - Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - }, - ExpectManagedFields{ - Manager: "default", - Fields: fieldpath.NewSet( + }, + Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, + Managed: fieldpath.ManagedFields{ + "default": fieldpath.NewVersionedSet( + fieldpath.NewSet( fieldpath.MakePathOrDie("numeric"), ), - }, + "v1", + true, + ), }, }, } From 570c5cb343fd89ba685f2c0e84f51a01945358a2 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 12:41:54 +0200 Subject: [PATCH 10/17] move RecursiveDifference code to SetNodeMap --- fieldpath/set.go | 82 +++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/fieldpath/set.go b/fieldpath/set.go index 5afc36ce..704f125f 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -102,43 +102,10 @@ func (s *Set) Difference(s2 *Set) *Set { // all children from s // that are either children or members in s2 func (s *Set) RecursiveDifference(s2 *Set) *Set { - out := &Set{ - Members: *s.Members.Difference(&s2.Members), - } - - i, j := 0, 0 - for i < len(s.Children.members) && j < len(s2.Children.members) { - if s.Children.members[i].pathElement.Less(s2.Children.members[j].pathElement) { - if !s2.Members.Has(s.Children.members[i].pathElement) { - out.Children.members = append(out.Children.members, setNode{ - pathElement: s.Children.members[i].pathElement, - set: s.Children.members[i].set, - }) - } - i++ - } else { - if !s2.Children.members[j].pathElement.Less(s.Children.members[i].pathElement) { - if !s2.Members.Has(s.Children.members[i].pathElement) { - diff := s.Children.members[i].set.RecursiveDifference(s2.Children.members[j].set) - if !diff.Empty() { - out.Children.members = append(out.Children.members, setNode{pathElement: s.Children.members[i].pathElement, set: diff}) - } - } - i++ - } - j++ - } - } - - if i < len(s.Children.members) { - for _, c := range s.Children.members[i:] { - if !s2.Members.Has(c.pathElement) { - out.Children.members = append(out.Children.members, c) - } - } + return &Set{ + Members: *s.Members.Difference(&s2.Members), + Children: *s.Children.RecursiveDifference(s2), } - - return out } // Size returns the number of members of the set. @@ -380,6 +347,49 @@ func (s *SetNodeMap) Difference(s2 *Set) *SetNodeMap { return out } +// RecursiveDifference returns a SetNodeMap with members that appear in s but not in s2. +// +// Compared to a regular difference, this recursively removes +// all children from s +// that are either children or members in s2 +func (s *SetNodeMap) RecursiveDifference(s2 *Set) *SetNodeMap { + out := &SetNodeMap{} + + i, j := 0, 0 + for i < len(s.members) && j < len(s2.Children.members) { + if s.members[i].pathElement.Less(s2.Children.members[j].pathElement) { + if !s2.Members.Has(s.members[i].pathElement) { + out.members = append(out.members, setNode{ + pathElement: s.members[i].pathElement, + set: s.members[i].set, + }) + } + i++ + } else { + if !s2.Children.members[j].pathElement.Less(s.members[i].pathElement) { + if !s2.Members.Has(s.members[i].pathElement) { + diff := s.members[i].set.RecursiveDifference(s2.Children.members[j].set) + if !diff.Empty() { + out.members = append(out.members, setNode{pathElement: s.members[i].pathElement, set: diff}) + } + } + i++ + } + j++ + } + } + + if i < len(s.members) { + for _, c := range s.members[i:] { + if !s2.Members.Has(c.pathElement) { + out.members = append(out.members, c) + } + } + } + + return out +} + // Iterate calls f for each PathElement in the set. func (s *SetNodeMap) Iterate(f func(PathElement)) { for _, n := range s.members { From d89132764fa2acfe2af3facbbfacf25e722c047c Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 12:50:31 +0200 Subject: [PATCH 11/17] add RecursiveDifference to benchmarks --- fieldpath/set_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 73f046ea..9d55127a 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -131,6 +131,12 @@ func BenchmarkFieldSet(b *testing.B) { randOperand().Difference(randOperand()) } }) + b.Run(fmt.Sprintf("recursive-difference-%v", here.size), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + randOperand().RecursiveDifference(randOperand()) + } + }) } } From 496703aa312906a5101a3f4787fe04a416fd1af3 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Tue, 12 May 2020 12:53:58 +0200 Subject: [PATCH 12/17] add missing copyright notice --- merge/ignore_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/merge/ignore_test.go b/merge/ignore_test.go index c8263e80..dadc1dd4 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package merge_test import ( From 2c92fd26d9c2dd1ba1ad3d91d10070914a6706e9 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Wed, 13 May 2020 18:36:10 +0200 Subject: [PATCH 13/17] switch to versioned ignored fields in updater --- fieldpath/versions.go | 20 ++++++ internal/fixture/state.go | 36 ++++++---- merge/ignore_test.go | 147 ++++++++++++++++++++++++++++++-------- merge/update.go | 22 +++--- 4 files changed, 175 insertions(+), 50 deletions(-) create mode 100644 fieldpath/versions.go diff --git a/fieldpath/versions.go b/fieldpath/versions.go new file mode 100644 index 00000000..5d2fdd64 --- /dev/null +++ b/fieldpath/versions.go @@ -0,0 +1,20 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fieldpath + +// SetVersions stores versions of a set +type SetVersions map[APIVersion]*Set diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 07ae1f5d..3f9f02a6 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -109,7 +109,7 @@ func (s *State) checkInit(version fieldpath.APIVersion) error { return nil } -func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string) error { +func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string) error { err := s.checkInit(version) if err != nil { return err @@ -118,7 +118,8 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, ignored, manager) + defer setIgnoredFieldsAndCleanup(s.Updater, ignored)() + newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, manager) if err != nil { return err } @@ -129,7 +130,7 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Update the current state with the passed in object -func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string) error { +func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err @@ -137,7 +138,7 @@ func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignor return s.UpdateObject(tv, version, ignored, manager) } -func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string, force bool) error { +func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string, force bool) error { err := s.checkInit(version) if err != nil { return err @@ -146,7 +147,8 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - new, managers, err := s.Updater.Apply(s.Live, tv, version, s.Managers, ignored, manager, force) + defer setIgnoredFieldsAndCleanup(s.Updater, ignored)() + new, managers, err := s.Updater.Apply(s.Live, tv, version, s.Managers, manager, force) if err != nil { return err } @@ -158,7 +160,7 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Apply the passed in object to the current state -func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored *fieldpath.Set, manager string, force bool) error { +func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string, force bool) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err @@ -235,7 +237,7 @@ type Apply struct { APIVersion fieldpath.APIVersion Object typed.YAMLObject Conflicts merge.Conflicts - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &Apply{} @@ -267,7 +269,7 @@ type ApplyObject struct { APIVersion fieldpath.APIVersion Object *typed.TypedValue Conflicts merge.Conflicts - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &ApplyObject{} @@ -306,7 +308,7 @@ type ForceApply struct { Manager string APIVersion fieldpath.APIVersion Object typed.YAMLObject - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &ForceApply{} @@ -334,7 +336,7 @@ type ForceApplyObject struct { Manager string APIVersion fieldpath.APIVersion Object *typed.TypedValue - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &ForceApplyObject{} @@ -353,7 +355,7 @@ type Update struct { Manager string APIVersion fieldpath.APIVersion Object typed.YAMLObject - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &Update{} @@ -381,7 +383,7 @@ type UpdateObject struct { Manager string APIVersion fieldpath.APIVersion Object *typed.TypedValue - IgnoredFields *fieldpath.Set + IgnoredFields fieldpath.SetVersions } var _ Operation = &Update{} @@ -514,3 +516,13 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e return nil } + +// setIgnoredFieldsAndCleanup sets the ignored fields for the provided updater and returns the cleanup function resetting them again +// this way it can be called in a single defer call +func setIgnoredFieldsAndCleanup(updater *merge.Updater, ignored fieldpath.SetVersions) func() { + if ignored == nil { + return func() {} + } + updater.IgnoredFields = ignored + return func() { updater.IgnoredFields = nil } +} diff --git a/merge/ignore_test.go b/merge/ignore_test.go index dadc1dd4..755bd622 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -35,9 +35,11 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "some string" `, - IgnoredFields: fieldpath.NewSet( - fieldpath.MakePathOrDie("string"), - ), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS( + _P("string"), + ), + }, }, }, Object: ` @@ -46,8 +48,8 @@ func TestIgnoredFields(t *testing.T) { `, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), + _NS( + _P("numeric"), ), "v1", false, @@ -72,7 +74,11 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "no string" `, - IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS( + _P("string"), + ), + }, }, }, Object: ` @@ -81,9 +87,9 @@ func TestIgnoredFields(t *testing.T) { `, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), - fieldpath.MakePathOrDie("string"), + _NS( + _P("numeric"), + _P("string"), ), "v1", false, @@ -97,16 +103,18 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: fieldpath.NewSet( - fieldpath.MakePathOrDie("obj"), - ), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS( + _P("obj"), + ), + }, }, }, Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), + _NS( + _P("numeric"), ), "v1", false, @@ -123,9 +131,11 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "some string" `, - IgnoredFields: fieldpath.NewSet( - fieldpath.MakePathOrDie("string"), - ), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS( + _P("string"), + ), + }, }, }, Object: ` @@ -134,8 +144,8 @@ func TestIgnoredFields(t *testing.T) { `, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), + _NS( + _P("numeric"), ), "v1", true, @@ -160,7 +170,9 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "no string" `, - IgnoredFields: fieldpath.NewSet(fieldpath.MakePathOrDie("string")), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS(_P("string")), + }, }, }, Object: ` @@ -169,16 +181,16 @@ func TestIgnoredFields(t *testing.T) { `, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), - fieldpath.MakePathOrDie("string"), + _NS( + _P("numeric"), + _P("string"), ), "v1", true, ), "default2": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), + _NS( + _P("numeric"), ), "v1", true, @@ -192,16 +204,18 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: fieldpath.NewSet( - fieldpath.MakePathOrDie("obj"), - ), + IgnoredFields: fieldpath.SetVersions{ + "v1": _NS( + _P("obj"), + ), + }, }, }, Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, Managed: fieldpath.ManagedFields{ "default": fieldpath.NewVersionedSet( - fieldpath.NewSet( - fieldpath.MakePathOrDie("numeric"), + _NS( + _P("numeric"), ), "v1", true, @@ -218,3 +232,76 @@ func TestIgnoredFields(t *testing.T) { }) } } + +func TestIgnoredFieldsUsesVersions(t *testing.T) { + ignored := fieldpath.SetVersions{ + "v1": _NS( + _P("mapOfMapsRecursive", "c"), + ), + "v2": _NS( + _P("mapOfMapsRecursive", "cc"), + ), + "v3": _NS( + _P("mapOfMapsRecursive", "ccc"), + ), + "v4": _NS( + _P("mapOfMapsRecursive", "cccc"), + ), + } + test := TestCase{ + Ops: []Operation{ + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + IgnoredFields: ignored, + }, + Apply{ + Manager: "apply-two", + Object: ` + mapOfMapsRecursive: + aa: + cc: + dd: + `, + APIVersion: "v2", + IgnoredFields: ignored, + }, + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + `, + APIVersion: "v4", + IgnoredFields: ignored, + }, + }, + // note that this still contains cccc due to ignored fields not being removed from the update result + Object: ` + mapOfMapsRecursive: + aaaa: + cccc: + dddd: + `, + APIVersion: "v4", + Managed: fieldpath.ManagedFields{ + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "aa"), + ), + "v2", + false, + ), + }, + } + + if err := test.TestWithConverter(nestedTypeParser, repeatingConverter{nestedTypeParser}); err != nil { + t.Fatal(err) + } +} diff --git a/merge/update.go b/merge/update.go index f3270081..a25bbf91 100644 --- a/merge/update.go +++ b/merge/update.go @@ -30,7 +30,8 @@ type Converter interface { // Updater is the object used to compute updated FieldSets and also // merge the object on Apply. type Updater struct { - Converter Converter + Converter Converter + IgnoredFields fieldpath.SetVersions enableUnions bool } @@ -41,7 +42,7 @@ func (s *Updater) EnableUnionFeature() { s.enableUnions = true } -func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, workflow string, force bool) (fieldpath.ManagedFields, *typed.Comparison, error) { +func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, workflow string, force bool) (fieldpath.ManagedFields, *typed.Comparison, error) { conflicts := fieldpath.ManagedFields{} removed := fieldpath.ManagedFields{} compare, err := oldObject.Compare(newObject) @@ -49,6 +50,8 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } + compare.ExcludeFields(s.IgnoredFields[version]) + versions := map[fieldpath.APIVersion]*typed.Comparison{ version: compare, } @@ -80,11 +83,10 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa if err != nil { return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } + compare.ExcludeFields(s.IgnoredFields[managerSet.APIVersion()]) versions[managerSet.APIVersion()] = compare } - compare.ExcludeFields(ignored) - conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added)) if !conflictSet.Empty() { conflicts[manager] = fieldpath.NewVersionedSet(conflictSet, managerSet.APIVersion(), false) @@ -121,7 +123,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa // that you intend to persist (after applying the patch if this is for a // PATCH call), and liveObject must be the original object (empty if // this is a CREATE call). -func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) { +func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) { var err error if s.enableUnions { newObject, err = liveObject.NormalizeUnions(newObject) @@ -130,13 +132,15 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp } } managers = shallowCopyManagers(managers) - managers, compare, err := s.update(liveObject, newObject, version, managers, ignored, manager, true) + managers, compare, err := s.update(liveObject, newObject, version, managers, manager, true) if err != nil { return nil, fieldpath.ManagedFields{}, err } if _, ok := managers[manager]; !ok { managers[manager] = fieldpath.NewVersionedSet(fieldpath.NewSet(), version, false) } + + ignored := s.IgnoredFields[version] if ignored == nil { ignored = fieldpath.NewSet() } @@ -155,7 +159,7 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp // well as the configuration that is applied. This will merge the object // and return it. If the object hasn't changed, nil is returned (the // managers can still have changed though). -func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, ignored *fieldpath.Set, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) { +func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) { managers = shallowCopyManagers(managers) var err error if s.enableUnions { @@ -179,6 +183,8 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err) } + + ignored := s.IgnoredFields[version] if ignored != nil { set = set.RecursiveDifference(ignored) // TODO: is this correct. If we don't remove from lastSet pruning might remove the fields? @@ -191,7 +197,7 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel if err != nil { return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to prune fields: %v", err) } - managers, compare, err := s.update(liveObject, newObject, version, managers, ignored, manager, force) + managers, compare, err := s.update(liveObject, newObject, version, managers, manager, force) if err != nil { return nil, fieldpath.ManagedFields{}, err } From 486ee0612e3de663f50699ca099f0208c925057f Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Wed, 13 May 2020 20:22:46 +0200 Subject: [PATCH 14/17] more cleanup --- fieldpath/versions.go | 20 -------------------- internal/fixture/state.go | 22 +++++++++++----------- merge/ignore_test.go | 14 +++++++------- merge/update.go | 9 +++------ typed/typed.go | 5 +++-- 5 files changed, 24 insertions(+), 46 deletions(-) delete mode 100644 fieldpath/versions.go diff --git a/fieldpath/versions.go b/fieldpath/versions.go deleted file mode 100644 index 5d2fdd64..00000000 --- a/fieldpath/versions.go +++ /dev/null @@ -1,20 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package fieldpath - -// SetVersions stores versions of a set -type SetVersions map[APIVersion]*Set diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 3f9f02a6..34bc87ca 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -109,7 +109,7 @@ func (s *State) checkInit(version fieldpath.APIVersion) error { return nil } -func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string) error { +func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string) error { err := s.checkInit(version) if err != nil { return err @@ -130,7 +130,7 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Update the current state with the passed in object -func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string) error { +func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err @@ -138,7 +138,7 @@ func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignor return s.UpdateObject(tv, version, ignored, manager) } -func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string, force bool) error { +func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string, force bool) error { err := s.checkInit(version) if err != nil { return err @@ -160,7 +160,7 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Apply the passed in object to the current state -func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored fieldpath.SetVersions, manager string, force bool) error { +func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string, force bool) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err @@ -237,7 +237,7 @@ type Apply struct { APIVersion fieldpath.APIVersion Object typed.YAMLObject Conflicts merge.Conflicts - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &Apply{} @@ -269,7 +269,7 @@ type ApplyObject struct { APIVersion fieldpath.APIVersion Object *typed.TypedValue Conflicts merge.Conflicts - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &ApplyObject{} @@ -308,7 +308,7 @@ type ForceApply struct { Manager string APIVersion fieldpath.APIVersion Object typed.YAMLObject - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &ForceApply{} @@ -336,7 +336,7 @@ type ForceApplyObject struct { Manager string APIVersion fieldpath.APIVersion Object *typed.TypedValue - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &ForceApplyObject{} @@ -355,7 +355,7 @@ type Update struct { Manager string APIVersion fieldpath.APIVersion Object typed.YAMLObject - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &Update{} @@ -383,7 +383,7 @@ type UpdateObject struct { Manager string APIVersion fieldpath.APIVersion Object *typed.TypedValue - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } var _ Operation = &Update{} @@ -519,7 +519,7 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e // setIgnoredFieldsAndCleanup sets the ignored fields for the provided updater and returns the cleanup function resetting them again // this way it can be called in a single defer call -func setIgnoredFieldsAndCleanup(updater *merge.Updater, ignored fieldpath.SetVersions) func() { +func setIgnoredFieldsAndCleanup(updater *merge.Updater, ignored map[fieldpath.APIVersion]*fieldpath.Set) func() { if ignored == nil { return func() {} } diff --git a/merge/ignore_test.go b/merge/ignore_test.go index 755bd622..d8f55557 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -35,7 +35,7 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "some string" `, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("string"), ), @@ -74,7 +74,7 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "no string" `, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("string"), ), @@ -103,7 +103,7 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("obj"), ), @@ -131,7 +131,7 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "some string" `, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("string"), ), @@ -170,7 +170,7 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "no string" `, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS(_P("string")), }, }, @@ -204,7 +204,7 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: fieldpath.SetVersions{ + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("obj"), ), @@ -234,7 +234,7 @@ func TestIgnoredFields(t *testing.T) { } func TestIgnoredFieldsUsesVersions(t *testing.T) { - ignored := fieldpath.SetVersions{ + ignored := map[fieldpath.APIVersion]*fieldpath.Set{ "v1": _NS( _P("mapOfMapsRecursive", "c"), ), diff --git a/merge/update.go b/merge/update.go index a25bbf91..edf8eee3 100644 --- a/merge/update.go +++ b/merge/update.go @@ -31,7 +31,7 @@ type Converter interface { // merge the object on Apply. type Updater struct { Converter Converter - IgnoredFields fieldpath.SetVersions + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set enableUnions bool } @@ -50,10 +50,8 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } - compare.ExcludeFields(s.IgnoredFields[version]) - versions := map[fieldpath.APIVersion]*typed.Comparison{ - version: compare, + version: compare.ExcludeFields(s.IgnoredFields[version]), } for manager, managerSet := range managers { @@ -83,8 +81,7 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa if err != nil { return nil, nil, fmt.Errorf("failed to compare objects: %v", err) } - compare.ExcludeFields(s.IgnoredFields[managerSet.APIVersion()]) - versions[managerSet.APIVersion()] = compare + versions[managerSet.APIVersion()] = compare.ExcludeFields(s.IgnoredFields[managerSet.APIVersion()]) } conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added)) diff --git a/typed/typed.go b/typed/typed.go index 09947cae..9a7ef9be 100644 --- a/typed/typed.go +++ b/typed/typed.go @@ -294,11 +294,12 @@ func (c *Comparison) String() string { // ExcludeFields fields from the compare recursively removes the fields // from the entire comparison -func (c *Comparison) ExcludeFields(fields *fieldpath.Set) { +func (c *Comparison) ExcludeFields(fields *fieldpath.Set) *Comparison { if fields == nil || fields.Empty() { - return + return c } c.Removed = c.Removed.RecursiveDifference(fields) c.Modified = c.Modified.RecursiveDifference(fields) c.Added = c.Added.RecursiveDifference(fields) + return c } From a8d38712610a59039e7f4856928c4a456bf4732c Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Thu, 14 May 2020 21:21:34 +0200 Subject: [PATCH 15/17] make ignoredFields part of TestCase --- internal/fixture/state.go | 109 +++++------ merge/ignore_test.go | 330 ++++++++++++++++++-------------- merge/obsolete_versions_test.go | 10 +- 3 files changed, 233 insertions(+), 216 deletions(-) diff --git a/internal/fixture/state.go b/internal/fixture/state.go index 34bc87ca..c275ce8d 100644 --- a/internal/fixture/state.go +++ b/internal/fixture/state.go @@ -109,7 +109,7 @@ func (s *State) checkInit(version fieldpath.APIVersion) error { return nil } -func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string) error { +func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, manager string) error { err := s.checkInit(version) if err != nil { return err @@ -118,7 +118,6 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - defer setIgnoredFieldsAndCleanup(s.Updater, ignored)() newObj, managers, err := s.Updater.Update(s.Live, tv, version, s.Managers, manager) if err != nil { return err @@ -130,15 +129,15 @@ func (s *State) UpdateObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Update the current state with the passed in object -func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string) error { +func (s *State) Update(obj typed.YAMLObject, version fieldpath.APIVersion, manager string) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err } - return s.UpdateObject(tv, version, ignored, manager) + return s.UpdateObject(tv, version, manager) } -func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string, force bool) error { +func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, manager string, force bool) error { err := s.checkInit(version) if err != nil { return err @@ -147,7 +146,6 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, if err != nil { return err } - defer setIgnoredFieldsAndCleanup(s.Updater, ignored)() new, managers, err := s.Updater.Apply(s.Live, tv, version, s.Managers, manager, force) if err != nil { return err @@ -160,12 +158,12 @@ func (s *State) ApplyObject(tv *typed.TypedValue, version fieldpath.APIVersion, } // Apply the passed in object to the current state -func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, ignored map[fieldpath.APIVersion]*fieldpath.Set, manager string, force bool) error { +func (s *State) Apply(obj typed.YAMLObject, version fieldpath.APIVersion, manager string, force bool) error { tv, err := s.Parser.Type(string(version)).FromYAML(FixTabsOrDie(obj)) if err != nil { return err } - return s.ApplyObject(tv, version, ignored, manager, force) + return s.ApplyObject(tv, version, manager, force) } // CompareLive takes a YAML string and returns the comparison with the @@ -233,11 +231,10 @@ func addedConflicts(one, other merge.Conflicts) merge.Conflicts { // conflict, the user can specify the expected conflicts. If conflicts // don't match, an error will occur. type Apply struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject - Conflicts merge.Conflicts - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject + Conflicts merge.Conflicts } var _ Operation = &Apply{} @@ -256,26 +253,24 @@ func (a Apply) preprocess(parser Parser) (Operation, error) { return nil, err } return ApplyObject{ - Manager: a.Manager, - APIVersion: a.APIVersion, - Object: tv, - Conflicts: a.Conflicts, - IgnoredFields: a.IgnoredFields, + Manager: a.Manager, + APIVersion: a.APIVersion, + Object: tv, + Conflicts: a.Conflicts, }, nil } type ApplyObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue - Conflicts merge.Conflicts - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue + Conflicts merge.Conflicts } var _ Operation = &ApplyObject{} func (a ApplyObject) run(state *State) error { - err := state.ApplyObject(a.Object, a.APIVersion, a.IgnoredFields, a.Manager, false) + err := state.ApplyObject(a.Object, a.APIVersion, a.Manager, false) if err != nil { if _, ok := err.(merge.Conflicts); !ok || a.Conflicts == nil { return err @@ -305,16 +300,15 @@ func (a ApplyObject) preprocess(parser Parser) (Operation, error) { // ForceApply is a type of operation. It is a forced-apply run by a // manager with a given object. Any error will be returned. type ForceApply struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject } var _ Operation = &ForceApply{} func (f ForceApply) run(state *State) error { - return state.Apply(f.Object, f.APIVersion, f.IgnoredFields, f.Manager, true) + return state.Apply(f.Object, f.APIVersion, f.Manager, true) } func (f ForceApply) preprocess(parser Parser) (Operation, error) { @@ -323,26 +317,24 @@ func (f ForceApply) preprocess(parser Parser) (Operation, error) { return nil, err } return ForceApplyObject{ - Manager: f.Manager, - APIVersion: f.APIVersion, - Object: tv, - IgnoredFields: f.IgnoredFields, + Manager: f.Manager, + APIVersion: f.APIVersion, + Object: tv, }, nil } // ForceApplyObject is a type of operation. It is a forced-apply run by // a manager with a given object. Any error will be returned. type ForceApplyObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue } var _ Operation = &ForceApplyObject{} func (f ForceApplyObject) run(state *State) error { - return state.ApplyObject(f.Object, f.APIVersion, f.IgnoredFields, f.Manager, true) + return state.ApplyObject(f.Object, f.APIVersion, f.Manager, true) } func (f ForceApplyObject) preprocess(parser Parser) (Operation, error) { @@ -352,16 +344,15 @@ func (f ForceApplyObject) preprocess(parser Parser) (Operation, error) { // Update is a type of operation. It is a controller type of // update. Errors are passed along. type Update struct { - Manager string - APIVersion fieldpath.APIVersion - Object typed.YAMLObject - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object typed.YAMLObject } var _ Operation = &Update{} func (u Update) run(state *State) error { - return state.Update(u.Object, u.APIVersion, u.IgnoredFields, u.Manager) + return state.Update(u.Object, u.APIVersion, u.Manager) } func (u Update) preprocess(parser Parser) (Operation, error) { @@ -370,26 +361,24 @@ func (u Update) preprocess(parser Parser) (Operation, error) { return nil, err } return UpdateObject{ - Manager: u.Manager, - APIVersion: u.APIVersion, - Object: tv, - IgnoredFields: u.IgnoredFields, + Manager: u.Manager, + APIVersion: u.APIVersion, + Object: tv, }, nil } // UpdateObject is a type of operation. It is a controller type of // update. Errors are passed along. type UpdateObject struct { - Manager string - APIVersion fieldpath.APIVersion - Object *typed.TypedValue - IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set + Manager string + APIVersion fieldpath.APIVersion + Object *typed.TypedValue } var _ Operation = &Update{} func (u UpdateObject) run(state *State) error { - return state.UpdateObject(u.Object, u.APIVersion, u.IgnoredFields, u.Manager) + return state.UpdateObject(u.Object, u.APIVersion, u.Manager) } func (f UpdateObject) preprocess(parser Parser) (Operation, error) { @@ -416,6 +405,8 @@ type TestCase struct { Managed fieldpath.ManagedFields // Set to true if the test case needs the union behavior enabled. RequiresUnions bool + // IgnoredFields containing the set to ignore for every version + IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set } // Test runs the test-case using the given parser and a dummy converter. @@ -447,7 +438,7 @@ func (tc TestCase) PreprocessOperations(parser Parser) error { // actually passes.. func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) error { state := State{ - Updater: &merge.Updater{Converter: converter}, + Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields}, Parser: parser, } if tc.RequiresUnions { @@ -467,7 +458,7 @@ func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) // TestWithConverter runs the test-case using the given parser and converter. func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) error { state := State{ - Updater: &merge.Updater{Converter: converter}, + Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields}, Parser: parser, } if tc.RequiresUnions { @@ -516,13 +507,3 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e return nil } - -// setIgnoredFieldsAndCleanup sets the ignored fields for the provided updater and returns the cleanup function resetting them again -// this way it can be called in a single defer call -func setIgnoredFieldsAndCleanup(updater *merge.Updater, ignored map[fieldpath.APIVersion]*fieldpath.Set) func() { - if ignored == nil { - return func() {} - } - updater.IgnoredFields = ignored - return func() { updater.IgnoredFields = nil } -} diff --git a/merge/ignore_test.go b/merge/ignore_test.go index d8f55557..1853e137 100644 --- a/merge/ignore_test.go +++ b/merge/ignore_test.go @@ -35,11 +35,6 @@ func TestIgnoredFields(t *testing.T) { numeric: 1 string: "some string" `, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("string"), - ), - }, }, }, Object: ` @@ -55,44 +50,9 @@ func TestIgnoredFields(t *testing.T) { false, ), }, - }, - "update_does_not_steal_ignored": { - APIVersion: "v1", - Ops: []Operation{ - Update{ - Manager: "default", - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - }, - Update{ - Manager: "default2", - APIVersion: "v1", - Object: ` - numeric: 1 - string: "no string" - `, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("string"), - ), - }, - }, - }, - Object: ` - numeric: 1 - string: "no string" - `, - Managed: fieldpath.ManagedFields{ - "default": fieldpath.NewVersionedSet( - _NS( - _P("numeric"), - _P("string"), - ), - "v1", - false, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("string"), ), }, }, @@ -103,11 +63,6 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("obj"), - ), - }, }, }, Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, @@ -120,39 +75,13 @@ func TestIgnoredFields(t *testing.T) { false, ), }, - }, - "apply_does_not_own_ignored": { - APIVersion: "v1", - Ops: []Operation{ - Apply{ - Manager: "default", - APIVersion: "v1", - Object: ` - numeric: 1 - string: "some string" - `, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("string"), - ), - }, - }, - }, - Object: ` - numeric: 1 - string: "some string" - `, - Managed: fieldpath.ManagedFields{ - "default": fieldpath.NewVersionedSet( - _NS( - _P("numeric"), - ), - "v1", - true, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("obj"), ), }, }, - "apply_does_not_steal_ignored": { + "apply_does_not_own_ignored": { APIVersion: "v1", Ops: []Operation{ Apply{ @@ -163,17 +92,6 @@ func TestIgnoredFields(t *testing.T) { string: "some string" `, }, - Apply{ - Manager: "default2", - APIVersion: "v1", - Object: ` - numeric: 1 - string: "no string" - `, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS(_P("string")), - }, - }, }, Object: ` numeric: 1 @@ -183,17 +101,14 @@ func TestIgnoredFields(t *testing.T) { "default": fieldpath.NewVersionedSet( _NS( _P("numeric"), - _P("string"), ), "v1", true, ), - "default2": fieldpath.NewVersionedSet( - _NS( - _P("numeric"), - ), - "v1", - true, + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("string"), ), }, }, @@ -204,11 +119,6 @@ func TestIgnoredFields(t *testing.T) { Manager: "default", APIVersion: "v1", Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, - IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("obj"), - ), - }, }, }, Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`, @@ -221,6 +131,11 @@ func TestIgnoredFields(t *testing.T) { true, ), }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("obj"), + ), + }, }, } @@ -234,74 +149,195 @@ func TestIgnoredFields(t *testing.T) { } func TestIgnoredFieldsUsesVersions(t *testing.T) { - ignored := map[fieldpath.APIVersion]*fieldpath.Set{ - "v1": _NS( - _P("mapOfMapsRecursive", "c"), - ), - "v2": _NS( - _P("mapOfMapsRecursive", "cc"), - ), - "v3": _NS( - _P("mapOfMapsRecursive", "ccc"), - ), - "v4": _NS( - _P("mapOfMapsRecursive", "cccc"), - ), - } - test := TestCase{ - Ops: []Operation{ - Apply{ - Manager: "apply-one", - Object: ` + tests := map[string]TestCase{ + "does_use_ignored_fields_versions": { + Ops: []Operation{ + Apply{ + Manager: "apply-one", + Object: ` mapOfMapsRecursive: a: b: c: d: `, - APIVersion: "v1", - IgnoredFields: ignored, - }, - Apply{ - Manager: "apply-two", - Object: ` + APIVersion: "v1", + }, + Apply{ + Manager: "apply-two", + Object: ` mapOfMapsRecursive: aa: cc: dd: `, - APIVersion: "v2", - IgnoredFields: ignored, - }, - Apply{ - Manager: "apply-one", - Object: ` + APIVersion: "v2", + }, + Apply{ + Manager: "apply-one", + Object: ` mapOfMapsRecursive: `, - APIVersion: "v4", - IgnoredFields: ignored, + APIVersion: "v4", + }, }, - }, - // note that this still contains cccc due to ignored fields not being removed from the update result - Object: ` + // note that this still contains cccc due to ignored fields not being removed from the update result + Object: ` mapOfMapsRecursive: aaaa: cccc: dddd: `, - APIVersion: "v4", - Managed: fieldpath.ManagedFields{ - "apply-two": fieldpath.NewVersionedSet( - _NS( - _P("mapOfMapsRecursive", "aa"), + APIVersion: "v4", + Managed: fieldpath.ManagedFields{ + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "aa"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v1": _NS( + _P("mapOfMapsRecursive", "c"), + ), + "v2": _NS( + _P("mapOfMapsRecursive", "cc"), + ), + "v3": _NS( + _P("mapOfMapsRecursive", "ccc"), + ), + "v4": _NS( + _P("mapOfMapsRecursive", "cccc"), + ), + }, + }, + "update_does_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Update{ + Manager: "update-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + }, + Update{ + Manager: "update-two", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + APIVersion: "v2", + }, + }, + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + Managed: fieldpath.ManagedFields{ + "update-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive"), + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "c"), + ), + "v1", + false, ), - "v2", - false, - ), + "update-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v2": _NS( + _P("mapOfMapsRecursive", "c"), + ), + }, + }, + "apply_does_not_steal_ignored": { + APIVersion: "v1", + Ops: []Operation{ + Apply{ + Manager: "apply-one", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + APIVersion: "v1", + }, + Apply{ + Manager: "apply-two", + Object: ` + mapOfMapsRecursive: + a: + b: + c: + e: + `, + APIVersion: "v2", + }, + }, + Object: ` + mapOfMapsRecursive: + a: + b: + c: + d: + `, + Managed: fieldpath.ManagedFields{ + "apply-one": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + _P("mapOfMapsRecursive", "c"), + _P("mapOfMapsRecursive", "c", "d"), + ), + "v1", + false, + ), + "apply-two": fieldpath.NewVersionedSet( + _NS( + _P("mapOfMapsRecursive", "a"), + _P("mapOfMapsRecursive", "a", "b"), + ), + "v2", + false, + ), + }, + IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{ + "v2": _NS( + _P("mapOfMapsRecursive", "c"), + ), + }, }, } - if err := test.TestWithConverter(nestedTypeParser, repeatingConverter{nestedTypeParser}); err != nil { - t.Fatal(err) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if err := test.TestWithConverter(nestedTypeParser, repeatingConverter{nestedTypeParser}); err != nil { + t.Fatal(err) + } + }) } } diff --git a/merge/obsolete_versions_test.go b/merge/obsolete_versions_test.go index 4ac2413b..751b63c3 100644 --- a/merge/obsolete_versions_test.go +++ b/merge/obsolete_versions_test.go @@ -56,16 +56,16 @@ func TestObsoleteVersions(t *testing.T) { Parser: DeducedParser, } - if err := state.Update(typed.YAMLObject(`{"v1": 0}`), fieldpath.APIVersion("v1"), nil, "v1"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0}`), fieldpath.APIVersion("v1"), "v1"); err != nil { t.Fatalf("Failed to apply: %v", err) } - if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0}`), fieldpath.APIVersion("v2"), nil, "v2"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0}`), fieldpath.APIVersion("v2"), "v2"); err != nil { t.Fatalf("Failed to apply: %v", err) } // Remove v1, add v3 instead. converter.AcceptedVersions = []fieldpath.APIVersion{"v2", "v3"} - if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0, "v3": 0}`), fieldpath.APIVersion("v3"), nil, "v3"); err != nil { + if err := state.Update(typed.YAMLObject(`{"v1": 0, "v2": 0, "v3": 0}`), fieldpath.APIVersion("v3"), "v3"); err != nil { t.Fatalf("Failed to apply: %v", err) } @@ -113,13 +113,13 @@ func TestApplyObsoleteVersion(t *testing.T) { Parser: SameVersionParser{T: parser.Type("sets")}, } - if err := state.Apply(typed.YAMLObject(`{"list": ["a", "b", "c", "d"]}`), fieldpath.APIVersion("v1"), nil, "apply", false); err != nil { + if err := state.Apply(typed.YAMLObject(`{"list": ["a", "b", "c", "d"]}`), fieldpath.APIVersion("v1"), "apply", false); err != nil { t.Fatalf("Failed to apply: %v", err) } // Remove v1, add v2 instead. converter.AcceptedVersions = []fieldpath.APIVersion{"v2"} - if err := state.Apply(typed.YAMLObject(`{"list": ["a"]}`), fieldpath.APIVersion("v2"), nil, "apply", false); err != nil { + if err := state.Apply(typed.YAMLObject(`{"list": ["a"]}`), fieldpath.APIVersion("v2"), "apply", false); err != nil { t.Fatalf("Failed to apply: %v", err) } From 73b21b9b057d8d47044a1b136cbab48ce077be4c Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Thu, 14 May 2020 21:28:13 +0200 Subject: [PATCH 16/17] add test --- fieldpath/set_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 9d55127a..3c88435b 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -498,6 +498,15 @@ func TestSetRecursiveDifference(t *testing.T) { MakePathOrDie("a", "ab", "aba"), ), }, + { + name: "remove all matching children", + a: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + b: NewSet(MakePathOrDie("a")), + expect: NewSet(), + }, } for _, c := range table { From 02d431a3e322967228e3839e4850650a0f3ebac5 Mon Sep 17 00:00:00 2001 From: Kevin Wiesmueller Date: Mon, 18 May 2020 19:32:28 +0200 Subject: [PATCH 17/17] update RecursiveDifference comment and extend tests --- fieldpath/set.go | 21 ++++++------ fieldpath/set_test.go | 75 ++++++++++++++++++++++++++++--------------- 2 files changed, 60 insertions(+), 36 deletions(-) diff --git a/fieldpath/set.go b/fieldpath/set.go index 704f125f..029c2b60 100644 --- a/fieldpath/set.go +++ b/fieldpath/set.go @@ -98,9 +98,11 @@ func (s *Set) Difference(s2 *Set) *Set { // * appear in s // * do not appear in s2 // -// Compared to a regular difference, this recursively removes -// all children from s -// that are either children or members in s2 +// Compared to a regular difference, +// this removes every field **and its children** from s that is contained in s2. +// +// For example, with s containing `a.b.c` and s2 containing `a.b`, +// a RecursiveDifference will result in `a`, as the entire node `a.b` gets removed. func (s *Set) RecursiveDifference(s2 *Set) *Set { return &Set{ Members: *s.Members.Difference(&s2.Members), @@ -349,9 +351,11 @@ func (s *SetNodeMap) Difference(s2 *Set) *SetNodeMap { // RecursiveDifference returns a SetNodeMap with members that appear in s but not in s2. // -// Compared to a regular difference, this recursively removes -// all children from s -// that are either children or members in s2 +// Compared to a regular difference, +// this removes every field **and its children** from s that is contained in s2. +// +// For example, with s containing `a.b.c` and s2 containing `a.b`, +// a RecursiveDifference will result in `a`, as the entire node `a.b` gets removed. func (s *SetNodeMap) RecursiveDifference(s2 *Set) *SetNodeMap { out := &SetNodeMap{} @@ -359,10 +363,7 @@ func (s *SetNodeMap) RecursiveDifference(s2 *Set) *SetNodeMap { for i < len(s.members) && j < len(s2.Children.members) { if s.members[i].pathElement.Less(s2.Children.members[j].pathElement) { if !s2.Members.Has(s.members[i].pathElement) { - out.members = append(out.members, setNode{ - pathElement: s.members[i].pathElement, - set: s.members[i].set, - }) + out.members = append(out.members, setNode{pathElement: s.members[i].pathElement, set: s.members[i].set}) } i++ } else { diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go index 3c88435b..8eee60c1 100644 --- a/fieldpath/set_test.go +++ b/fieldpath/set_test.go @@ -453,24 +453,27 @@ func TestSetIntersectionDifference(t *testing.T) { }) } -func TestSetRecursiveDifference(t *testing.T) { +func TestSetDifference(t *testing.T) { table := []struct { - name string - a *Set - b *Set - expect *Set + name string + a *Set + b *Set + expectDifference *Set + expectRecursiveDifference *Set }{ { - name: "removes simple path", - a: NewSet(MakePathOrDie("a")), - b: NewSet(MakePathOrDie("a")), - expect: NewSet(), + name: "removes simple path", + a: NewSet(MakePathOrDie("a")), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet(), + expectRecursiveDifference: NewSet(), }, { - name: "removes direct path", - a: NewSet(MakePathOrDie("a", "b", "c")), - b: NewSet(MakePathOrDie("a", "b", "c")), - expect: NewSet(), + name: "removes direct path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a", "b", "c")), + expectDifference: NewSet(), + expectRecursiveDifference: NewSet(), }, { name: "only removes matching child", @@ -478,41 +481,61 @@ func TestSetRecursiveDifference(t *testing.T) { MakePathOrDie("a", "b", "c"), MakePathOrDie("b", "b", "c"), ), - b: NewSet(MakePathOrDie("a", "b", "c")), - expect: NewSet(MakePathOrDie("b", "b", "c")), + b: NewSet(MakePathOrDie("a", "b", "c")), + expectDifference: NewSet(MakePathOrDie("b", "b", "c")), + expectRecursiveDifference: NewSet(MakePathOrDie("b", "b", "c")), }, { - name: "removes nested path", - a: NewSet(MakePathOrDie("a", "b", "c")), - b: NewSet(MakePathOrDie("a")), - expect: NewSet(), + name: "does not remove parent of specific path", + a: NewSet( + MakePathOrDie("a"), + ), + b: NewSet(MakePathOrDie("a", "aa")), + expectDifference: NewSet(MakePathOrDie("a")), + expectRecursiveDifference: NewSet(MakePathOrDie("a")), }, { - name: "only removes nested path for matching children", + name: "RecursiveDifference removes nested path", + a: NewSet(MakePathOrDie("a", "b", "c")), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet(MakePathOrDie("a", "b", "c")), + expectRecursiveDifference: NewSet(), + }, + { + name: "RecursiveDifference only removes nested path for matching children", a: NewSet( MakePathOrDie("a", "aa", "aab"), MakePathOrDie("a", "ab", "aba"), ), b: NewSet(MakePathOrDie("a", "aa")), - expect: NewSet( + expectDifference: NewSet( + MakePathOrDie("a", "aa", "aab"), MakePathOrDie("a", "ab", "aba"), ), + expectRecursiveDifference: NewSet(MakePathOrDie("a", "ab", "aba")), }, { - name: "remove all matching children", + name: "RecursiveDifference removes all matching children", a: NewSet( MakePathOrDie("a", "aa", "aab"), MakePathOrDie("a", "ab", "aba"), ), - b: NewSet(MakePathOrDie("a")), - expect: NewSet(), + b: NewSet(MakePathOrDie("a")), + expectDifference: NewSet( + MakePathOrDie("a", "aa", "aab"), + MakePathOrDie("a", "ab", "aba"), + ), + expectRecursiveDifference: NewSet(), }, } for _, c := range table { t.Run(c.name, func(t *testing.T) { - if result := c.a.RecursiveDifference(c.b); !result.Equals(c.expect) { - t.Fatalf("expected: \n%v\n, got: \n%v\n", c.expect, result) + if result := c.a.Difference(c.b); !result.Equals(c.expectDifference) { + t.Fatalf("Difference expected: \n%v\n, got: \n%v\n", c.expectDifference, result) + } + if result := c.a.RecursiveDifference(c.b); !result.Equals(c.expectRecursiveDifference) { + t.Fatalf("RecursiveDifference expected: \n%v\n, got: \n%v\n", c.expectRecursiveDifference, result) } }) }