Cherry pick 166 184 #190

apelisse · 2021-03-25T21:51:55Z

This is a not-clean cherry-pick of #166 and #184.

I also had to add the missing Schema function added in another PR but used in #184. Very minor though.

…ther owners

Co-authored-by: Antoine Pelisse <[email protected]>

k8s-ci-robot · 2021-03-25T21:52:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [apelisse]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2021-03-26T00:30:32Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.
/check-cla

apelisse · 2021-03-29T17:53:32Z

OK I'm diffing the difference introduced by the actual PR and the cherry-pick. Diffing diffs is not a common thing so ⚠️ :

For #166, diff of diffs only shows different line numbers and index hashes, looks good 👍

apelisse ~/code/smd> diff -u first-pr-initial first-pr-cherry-pick 
--- first-pr-initial	2021-03-29 10:41:51.000000000 -0700
+++ first-pr-cherry-pick	2021-03-29 10:42:24.000000000 -0700
@@ -716,10 +716,10 @@
  	test := TestCase{
  		Ops: []Operation{
 diff --git a/merge/update.go b/merge/update.go
-index edf8eee..7b88cbd 100644
+index 3c2a469..c410a9a 100644
 --- a/merge/update.go
 +++ b/merge/update.go
-@@ -212,7 +212,7 @@ func shallowCopyManagers(managers fieldpath.ManagedFields) fieldpath.ManagedFiel
+@@ -197,7 +197,7 @@ func shallowCopyManagers(managers fieldpath.ManagedFields) fieldpath.ManagedFiel
  	return newManagers
  }
  
@@ -728,7 +728,7 @@
  // * applyingManager applied it last time
  // * applyingManager didn't apply it this time
  // * no other applier claims to manage it
-@@ -240,18 +240,16 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel
+@@ -225,18 +225,16 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel
  	return s.Converter.Convert(pruned, managers[applyingManager].APIVersion())
  }
  
@@ -752,7 +752,7 @@
  	}
  	for version, managed := range managedAtVersion {
  		merged, err = s.Converter.Convert(merged, version)
-@@ -281,9 +279,9 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie
+@@ -266,9 +264,9 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie
  	return pruned, nil
  }
  
@@ -784,7 +784,7 @@
  		if subset := w.toRemove.WithPrefix(pe); !subset.Empty() {
  			val = removeItemsWithSchema(val, subset, w.schema, fieldType)
 diff --git a/typed/typed.go b/typed/typed.go
-index 9a7ef9b..056a07d 100644
+index 4aa9a23..b8fea9e 100644
 --- a/typed/typed.go
 +++ b/typed/typed.go
 @@ -61,6 +61,11 @@ type TypedValue struct {

For 184, it's a little more messy, but basically nothing dramatic beyond the new function as mentioned in the initial comment:

--- 184-actual	2021-03-29 10:48:28.000000000 -0700
+++ 184-cp	2021-03-29 10:48:49.000000000 -0700
@@ -1,5 +1,5 @@
 diff --git a/fieldpath/set.go b/fieldpath/set.go
-index 029c2b6..5461326 100644
+index f280a2f..873b6b2 100644
 --- a/fieldpath/set.go
 +++ b/fieldpath/set.go
 @@ -19,6 +19,8 @@ package fieldpath
@@ -7,11 +7,11 @@
  	"sort"
  	"strings"
 +
-+	"sigs.k8s.io/structured-merge-diff/v4/schema"
++	"sigs.k8s.io/structured-merge-diff/v3/schema"
  )
  
  // Set identifies a set of fields.
-@@ -110,6 +112,30 @@ func (s *Set) RecursiveDifference(s2 *Set) *Set {
+@@ -94,6 +96,30 @@ func (s *Set) Difference(s2 *Set) *Set {
  	}
  }
  
@@ -42,7 +42,7 @@
  // Size returns the number of members of the set.
  func (s *Set) Size() int {
  	return s.Members.Size() + s.Children.Size()
-@@ -391,6 +417,31 @@ func (s *SetNodeMap) RecursiveDifference(s2 *Set) *SetNodeMap {
+@@ -333,6 +359,31 @@ func (s *SetNodeMap) Difference(s2 *Set) *SetNodeMap {
  	return out
  }
  
@@ -75,7 +75,7 @@
  func (s *SetNodeMap) Iterate(f func(PathElement)) {
  	for _, n := range s.members {
 diff --git a/fieldpath/set_test.go b/fieldpath/set_test.go
-index 8eee60c..b32b8cf 100644
+index fb06dd0..97bd8ed 100644
 --- a/fieldpath/set_test.go
 +++ b/fieldpath/set_test.go
 @@ -21,6 +21,9 @@ import (
@@ -84,12 +84,12 @@
  	"testing"
 +
 +	"gopkg.in/yaml.v2"
-+	"sigs.k8s.io/structured-merge-diff/v4/schema"
++	"sigs.k8s.io/structured-merge-diff/v3/schema"
  )
  
  type randomPathAlphabet []PathElement
-@@ -541,6 +544,86 @@ func TestSetDifference(t *testing.T) {
- 	}
+@@ -447,6 +450,86 @@ func TestSetIntersectionDifference(t *testing.T) {
+ 	})
  }
  
 +var nestedSchema = func() (*schema.Schema, schema.TypeRef) {
@@ -175,26 +175,8 @@
  func TestSetNodeMapIterate(t *testing.T) {
  	set := &SetNodeMap{}
  	toAdd := 5
-diff --git a/merge/ignore_test.go b/merge/ignore_test.go
-index 21b3c84..4cf7a91 100644
---- a/merge/ignore_test.go
-+++ b/merge/ignore_test.go
-@@ -190,6 +190,13 @@ func TestIgnoredFieldsUsesVersions(t *testing.T) {
- 			`,
- 			APIVersion: "v4",
- 			Managed: fieldpath.ManagedFields{
-+				"apply-one": fieldpath.NewVersionedSet(
-+					_NS(
-+						_P("mapOfMapsRecursive"),
-+					),
-+					"v4",
-+					false,
-+				),
- 				"apply-two": fieldpath.NewVersionedSet(
- 					_NS(
- 						_P("mapOfMapsRecursive", "aa"),
 diff --git a/merge/multiple_appliers_test.go b/merge/multiple_appliers_test.go
-index ae4706a..d90d9a0 100644
+index fcd1e8a..c879f30 100644
 --- a/merge/multiple_appliers_test.go
 +++ b/merge/multiple_appliers_test.go
 @@ -554,11 +554,10 @@ func testMultipleAppliersFieldUnsetting(t *testing.T, v1, v2, v3 fieldpath.APIVe
@@ -240,7 +222,7 @@
  					_NS(
  						_P("mapOfMapsRecursive", "aa"),
 diff --git a/merge/nested_test.go b/merge/nested_test.go
-index 367adca..c87c46b 100644
+index 0bb1ef5..ce7308e 100644
 --- a/merge/nested_test.go
 +++ b/merge/nested_test.go
 @@ -44,6 +44,18 @@ var nestedTypeParser = func() Parser {
@@ -457,10 +439,10 @@
  
  	for name, test := range tests {
 diff --git a/merge/update.go b/merge/update.go
-index 73e0ec7..33a3085 100644
+index c410a9a..f4fa5ed 100644
 --- a/merge/update.go
 +++ b/merge/update.go
-@@ -226,7 +226,8 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel
+@@ -213,7 +213,8 @@ func (s *Updater) prune(merged *typed.TypedValue, managers fieldpath.ManagedFiel
  		return nil, fmt.Errorf("failed to convert merged object to last applied version: %v", err)
  	}
  
@@ -470,7 +452,7 @@
  	pruned, err = s.addBackOwnedItems(convertedMerged, pruned, managers, applyingManager)
  	if err != nil {
  		return nil, fmt.Errorf("failed add back owned items: %v", err)
-@@ -272,7 +273,8 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie
+@@ -259,7 +260,8 @@ func (s *Updater) addBackOwnedItems(merged, pruned *typed.TypedValue, managedFie
  		if err != nil {
  			return nil, fmt.Errorf("failed to create field set from pruned object at version %v: %v", version, err)
  		}
@@ -480,7 +462,7 @@
  	}
  	return pruned, nil
  }
-@@ -296,7 +298,11 @@ func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet
+@@ -283,5 +285,9 @@ func (s *Updater) addBackDanglingItems(merged, pruned *typed.TypedValue, lastSet
  	if err != nil {
  		return nil, fmt.Errorf("failed to create field set from merged object in last applied version: %v", err)
  	}
@@ -491,10 +473,8 @@
 +	last := lastSet.Set().EnsureNamedFieldsAreMembers(sc, tr)
 +	return merged.RemoveItems(mergedSet.Difference(prunedSet).Intersection(last)), nil
  }
- 
- // reconcileManagedFieldsWithSchemaChanges reconciles the managed fields with any changes to the
 diff --git a/typed/tofieldset.go b/typed/tofieldset.go
-index bc88c08..047efff 100644
+index b3c4ff0..0800a9d 100644
 --- a/typed/tofieldset.go
 +++ b/typed/tofieldset.go
 @@ -137,7 +137,9 @@ func (v *toFieldSetWalker) visitMapItems(t *schema.Map, m value.Map) (errs Valid
@@ -508,3 +488,19 @@
  			v2.set.Insert(v2.path)
  		}
  		v.finishDescent(v2)
+diff --git a/typed/typed.go b/typed/typed.go
+index b8fea9e..ae4d83f 100644
+--- a/typed/typed.go
++++ b/typed/typed.go
+@@ -71,6 +71,11 @@ func (tv TypedValue) AsValue() value.Value {
+ 	return tv.value
+ }
+ 
++// Schema gets the schema from the TypedValue.
++func (tv TypedValue) Schema() *schema.Schema {
++	return tv.schema
++}
++
+ // Validate returns an error with a list of every spec violation.
+ func (tv TypedValue) Validate() error {
+ 	w := tv.walker()

lavalamp · 2021-03-29T18:59:04Z

Can we really back-port a name change from Difference to RecusiveDifference? Wouldn't that violate semver?

lavalamp · 2021-03-29T19:03:34Z

Looks fine apart from that name, thanks for the 2nd diffrivative ;)

apelisse · 2021-03-29T19:20:14Z

Can we really back-port a name change from Difference to RecusiveDifference? Wouldn't that violate semver?

It's not doing that. It's just that the surrounding of the diff have changed, the diff of diffs is very confusing :-)

lavalamp · 2021-03-29T22:29:33Z

oh, that's very confusing. OK, well, I looked at the net difference and that seems fine.

/lgtm

jpbetz and others added 10 commits March 25, 2021 14:41

Remove fields when unset from applied configuration if there are no o…

c7239f7

…ther owners

Add field unsetting tests

173a95e

Apply suggestion

e96f1eb

Co-authored-by: Antoine Pelisse <[email protected]>

Change merge rules so that updaters to share ownership with appliers

95a763d

Add and fix tests to match improved update merging

660a7a3

Add missing TypedValue.Schema function

8035048

Create NestedSet to populate set branches along with the leaves

35bfbd9

Use NestedSet to also remove structs owned by no-one

6611ec9

Fix tests broken by new mechanism

928ab00

Show how null is inserted when struct is removed

65b8000

k8s-ci-robot requested review from jennybuckley and lavalamp March 25, 2021 21:52

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 25, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2021

k8s-ci-robot assigned lavalamp Mar 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2021

k8s-ci-robot merged commit 7cc0d63 into kubernetes-sigs:release-3.0 Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cherry pick 166 184 #190

Cherry pick 166 184 #190

Uh oh!

apelisse commented Mar 25, 2021

Uh oh!

k8s-ci-robot commented Mar 25, 2021

Uh oh!

fejta-bot commented Mar 26, 2021

Uh oh!

apelisse commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

apelisse commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cherry pick 166 184 #190

Cherry pick 166 184 #190

Uh oh!

Conversation

apelisse commented Mar 25, 2021

Uh oh!

k8s-ci-robot commented Mar 25, 2021

Uh oh!

fejta-bot commented Mar 26, 2021

Uh oh!

apelisse commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

apelisse commented Mar 29, 2021

Uh oh!

lavalamp commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants