Skip to content

Commit

Permalink
Merge pull request #269 from jpbetz/fix-ignore-fields-break
Browse files Browse the repository at this point in the history 
Compatibility fix: Add back IgnoredFields
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Nov 14, 2024
2 parents ccf7a06 + adaddb2 commit db46cc3
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 6 deletions.
6 changes: 6 additions & 0 deletions internal/fixture/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,10 @@ type TestCase struct {
ReturnInputOnNoop bool
// IgnoreFilter filters out ignored fields from a fieldpath.Set.
IgnoreFilter map[fieldpath.APIVersion]fieldpath.Filter

// IgnoredFields containing the set to ignore for every version.
// IgnoredFields may not be set if IgnoreFilter is set.
IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
}

// Test runs the test-case using the given parser and a dummy converter.
Expand Down Expand Up @@ -573,6 +577,7 @@ func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter)
updaterBuilder := merge.UpdaterBuilder{
Converter: converter,
IgnoreFilter: tc.IgnoreFilter,
IgnoredFields: tc.IgnoredFields,
ReturnInputOnNoop: tc.ReturnInputOnNoop,
}
state := State{
Expand All @@ -595,6 +600,7 @@ func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) e
updaterBuilder := merge.UpdaterBuilder{
Converter: converter,
IgnoreFilter: tc.IgnoreFilter,
IgnoredFields: tc.IgnoredFields,
ReturnInputOnNoop: tc.ReturnInputOnNoop,
}
state := State{
Expand Down
127 changes: 126 additions & 1 deletion merge/ignore_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
. "sigs.k8s.io/structured-merge-diff/v4/internal/fixture"
)

func TestIgnoredFields(t *testing.T) {
func TestIgnoreFilter(t *testing.T) {
tests := map[string]TestCase{
"update_does_not_own_ignored": {
APIVersion: "v1",
Expand Down Expand Up @@ -148,6 +148,131 @@ func TestIgnoredFields(t *testing.T) {
}
}

func TestIgnoredFields(t *testing.T) {
tests := map[string]TestCase{
"update_does_not_own_ignored": {
APIVersion: "v1",
Ops: []Operation{
Update{
Manager: "default",
APIVersion: "v1",
Object: `
numeric: 1
string: "some string"
`,
},
},
Object: `
numeric: 1
string: "some string"
`,
Managed: fieldpath.ManagedFields{
"default": fieldpath.NewVersionedSet(
_NS(
_P("numeric"),
),
"v1",
false,
),
},
IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{
"v1": _NS(
_P("string"),
),
},
},
"update_does_not_own_deep_ignored": {
APIVersion: "v1",
Ops: []Operation{
Update{
Manager: "default",
APIVersion: "v1",
Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`,
},
},
Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`,
Managed: fieldpath.ManagedFields{
"default": fieldpath.NewVersionedSet(
_NS(
_P("numeric"),
),
"v1",
false,
),
},
IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{
"v1": _NS(
_P("obj"),
),
},
},
"apply_does_not_own_ignored": {
APIVersion: "v1",
Ops: []Operation{
Apply{
Manager: "default",
APIVersion: "v1",
Object: `
numeric: 1
string: "some 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("string"),
),
},
},
"apply_does_not_own_deep_ignored": {
APIVersion: "v1",
Ops: []Operation{
Apply{
Manager: "default",
APIVersion: "v1",
Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`,
},
},
Object: `{"numeric": 1, "obj": {"string": "foo", "numeric": 2}}`,
Managed: fieldpath.ManagedFields{
"default": fieldpath.NewVersionedSet(
_NS(
_P("numeric"),
),
"v1",
true,
),
},
IgnoredFields: map[fieldpath.APIVersion]*fieldpath.Set{
"v1": _NS(
_P("obj"),
),
},
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
if err := test.Test(DeducedParser); err != nil {
t.Fatal("Should fail:", err)
}
})
}
}

func TestFilteredFieldsUsesVersions(t *testing.T) {
tests := map[string]TestCase{
"does_use_ignored_fields_versions": {
Expand Down
50 changes: 45 additions & 5 deletions merge/update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ type UpdaterBuilder struct {
Converter Converter
IgnoreFilter map[fieldpath.APIVersion]fieldpath.Filter

// IgnoredFields provides a set of fields to ignore for each
IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set

// Stop comparing the new object with old object after applying.
// This was initially used to avoid spurious etcd update, but
// since that's vastly inefficient, we've come-up with a better
Expand All @@ -46,6 +49,7 @@ func (u *UpdaterBuilder) BuildUpdater() *Updater {
return &Updater{
Converter: u.Converter,
IgnoreFilter: u.IgnoreFilter,
IgnoredFields: u.IgnoredFields,
returnInputOnNoop: u.ReturnInputOnNoop,
}
}
Expand All @@ -56,6 +60,9 @@ type Updater struct {
// Deprecated: This will eventually become private.
Converter Converter

// Deprecated: This will eventually become private.
IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set

// Deprecated: This will eventually become private.
IgnoreFilter map[fieldpath.APIVersion]fieldpath.Filter

Expand All @@ -70,8 +77,19 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
return nil, nil, fmt.Errorf("failed to compare objects: %v", err)
}

versions := map[fieldpath.APIVersion]*typed.Comparison{
version: compare.FilterFields(s.IgnoreFilter[version]),
var versions map[fieldpath.APIVersion]*typed.Comparison

if s.IgnoredFields != nil && s.IgnoreFilter != nil {
return nil, nil, fmt.Errorf("IgnoreFilter and IgnoreFilter may not both be set")
}
if s.IgnoredFields != nil {
versions = map[fieldpath.APIVersion]*typed.Comparison{
version: compare.ExcludeFields(s.IgnoredFields[version]),
}
} else {
versions = map[fieldpath.APIVersion]*typed.Comparison{
version: compare.FilterFields(s.IgnoreFilter[version]),
}
}

for manager, managerSet := range managers {
Expand Down Expand Up @@ -101,7 +119,12 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
if err != nil {
return nil, nil, fmt.Errorf("failed to compare objects: %v", err)
}
versions[managerSet.APIVersion()] = compare.FilterFields(s.IgnoreFilter[managerSet.APIVersion()])

if s.IgnoredFields != nil {
versions[managerSet.APIVersion()] = compare.ExcludeFields(s.IgnoredFields[managerSet.APIVersion()])
} else {
versions[managerSet.APIVersion()] = compare.FilterFields(s.IgnoreFilter[managerSet.APIVersion()])
}
}

conflictSet := managerSet.Set().Intersection(compare.Modified.Union(compare.Added))
Expand Down Expand Up @@ -154,7 +177,16 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
managers[manager] = fieldpath.NewVersionedSet(fieldpath.NewSet(), version, false)
}
set := managers[manager].Set().Difference(compare.Removed).Union(compare.Modified).Union(compare.Added)
ignoreFilter := s.IgnoreFilter[version]

if s.IgnoredFields != nil && s.IgnoreFilter != nil {
return nil, nil, fmt.Errorf("IgnoreFilter and IgnoreFilter may not both be set")
}
var ignoreFilter fieldpath.Filter
if s.IgnoredFields != nil {
ignoreFilter = fieldpath.NewExcludeSetFilter(s.IgnoredFields[version])
} else {
ignoreFilter = s.IgnoreFilter[version]
}
if ignoreFilter != nil {
set = ignoreFilter.Filter(set)
}
Expand Down Expand Up @@ -189,7 +221,15 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel
return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to get field set: %v", err)
}

ignoreFilter := s.IgnoreFilter[version]
if s.IgnoredFields != nil && s.IgnoreFilter != nil {
return nil, nil, fmt.Errorf("IgnoreFilter and IgnoreFilter may not both be set")
}
var ignoreFilter fieldpath.Filter
if s.IgnoredFields != nil {
ignoreFilter = fieldpath.NewExcludeSetFilter(s.IgnoredFields[version])
} else {
ignoreFilter = s.IgnoreFilter[version]
}
if ignoreFilter != nil {
set = ignoreFilter.Filter(set)
}
Expand Down

0 comments on commit db46cc3

Please sign in to comment.