add extended remove method for typed values #133

kwiesmueller · 2019-12-09T15:42:18Z

While working on an implementation of kubernetes/enhancements#1123
i discovered that removing fields is not behaving like I expected.

Provided an original fieldSet like:

.status.phase

and a fieldSet to be removed:

.status

The current Difference and Intersect methods for fieldSets do not match all child paths when a parent path is defined, because .status != .status.phase.

For most cases, this seems reasonable (like ownership).

But to define a set of fields that should be removed from the object, it is impractical.
To avoid a user has to define every deep path to be removed (for status wiping),
a second Remove method is added to value.TypedValue that removes a fieldpath once found.

It extends the already available RemoveItems behavior to also remove from maps.

typed/remove.go

typed/remove_test.go

value/value.go

typed/typed.go

kwiesmueller · 2019-12-23T20:11:57Z

/retest

apelisse

Thanks for adding the tests Kevin! They are extremely useful

typed/remove.go

value/value.go

apelisse · 2020-01-08T00:26:30Z

typed/remove_test.go

+			object: `{"a": "value", "b":{"c":"value"}}`,
+			remove: fieldpath.NewSet(fieldpath.MakePathOrDie("b")),
+			deep:   false,
+			// TODO: this seems to behave differently with a non deduced schema: {"a": "value", "b":{"c":"value"}} which is the reason for this PR


@jennybuckley probably knows, I suspect it's because we treat deduced fields as map fields?

I'm thinking about adding a second test that defines the schema for all the tests above to verify.
If the behavior differs, we can at least be sure we want it to do.
Probably tomorrow.

Yes I think this is because deduced schema would see "b":{"c":"value"} as a map item, and so it would be removable, but if you defined a schema which has the fields named, then it would be seen as a struct field, and I don't think we expect to remove those with RemoveItems, since this function was created to remove list/map items that no appliers owns anymore, a behavior which doesn't apply to struct fields (since we didn't want removing replicas from a config and reapplying to reset replicas to 1)

typed/remove_test.go

kwiesmueller · 2020-01-10T22:00:11Z

/retest
to see if everything breaks now (as it should with the race test)

kwiesmueller · 2020-01-10T22:00:54Z

/test pull-structured-merge-diff-test

typed/remove.go

go.mod

typed/remove.go

value/value.go

typed/typed.go

typed/remove_test.go

kwiesmueller · 2020-02-25T20:58:07Z

@apelisse i pushed my current state including the failing tests.
The TestUpdateLeaf ones are the ones i mentioned a week ago. They fail because of pruning and the new remove.

The TestMultipleAppliers are new and should be caused by the different null behavior.
I am not sure yet why they fail, but am looking at it.

apelisse

I haven't looked at merge/update.go yet and how prune will be broken with this. We can do that later.

apelisse · 2020-02-25T23:23:02Z

fieldpath/element.go

+}
+
+// NewIndexPathElement creates a PathElement with the given index
+func NewIndexPathElement(index int) PathElement {


You're not using the method above, but you're using this one. We really shouldn't depend on this. Let's see ...

apelisse · 2020-02-26T18:50:08Z

internal/fixture/state.go

 }
+
+// PrintState is an Operation printing the current state to help with debugging tests
+type PrintState struct{}


These are good suggestions. I had thought about some version of this but thought that it was equally simple to have multiple tests:

Test 1: do a Test 2: do a and b Test 3: do a, b and c

if you really care about intermediate state, what do you think?

Yes, multiple tests would solve that as well.
While creating (and debugging) the remove tests, it was already getting less comfortable working with them due to the amount of tests. Also because of my unfortunate decision not to name them...

For now I only wanted to understand and debug the broken tests that already exist.
Pulling them apart into multiple tests would have been something for another PR and then still might cause more clutter while the Print and Expect Operations would allow for the same result in the existing test fixture thing.

typed/remove.go

typed/remove_test.go

apelisse · 2020-02-26T20:15:36Z

merge/multiple_appliers_test.go

 			Object: `
 				mapOfMapsRecursive:
-				  a:
+				  a: {}


This is a little unfortunate :-/ but I think that's fine.

apelisse

As far as I can tell, the code looked OK to me, and I don't see anything wrong with the tests either. I suspect we can try a few more corner-cases though.

internal/fixture/state.go

apelisse · 2020-03-06T19:17:55Z

internal/fixture/state.go

+			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))


Maybe move this code to a separate function since it's done twice and is hard to read.

typed/remove_test.go

apelisse · 2020-03-06T20:09:47Z

typed/remove_test.go

+		},
+		{
+			object:          `{"a": "value", "b": ["c", "d"]}`,
+			remove:          _NS(_P("b", _I(1))),


I don't see a test that removes by value, do we have one? Maybe below, but I suspect it belongs here

typed/remove_test.go

apelisse · 2020-03-06T23:04:46Z

typed/remove.go

 	l := w.value.AsListUsing(w.allocator)
 	defer w.allocator.Free(l)
-	// If list is null, empty, or atomic just return
-	if l == nil || l.Length() == 0 || t.ElementRelationship == schema.Atomic {


@jennybuckley Do you remember why we had the atomic check here? Would it be a problem to remove it?

apelisse

OK I think this is good, @jennybuckley would you mind looking at this?

internal/fixture/state.go

jennybuckley · 2020-03-17T16:43:54Z

/assign

k8s-ci-robot · 2020-03-17T17:11:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kwiesmueller
To complete the pull request process, please assign jennybuckley
You can assign the PR to them by writing /assign @jennybuckley in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

kwiesmueller · 2020-04-10T14:14:22Z

@jennybuckley did you have time to look at this?

apelisse · 2020-04-10T17:01:39Z

Can you please squash? Thanks

fieldpath/element.go

… of fields

fejta-bot · 2020-07-09T17:42:04Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-07-09T17:42:12Z

@kwiesmueller: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

apelisse · 2020-07-09T17:51:59Z

I think that's been replaced.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 9, 2019

k8s-ci-robot requested review from apelisse and jennybuckley December 9, 2019 15:42

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 9, 2019

kwiesmueller commented Dec 9, 2019

View reviewed changes

typed/remove.go Outdated Show resolved Hide resolved

typed/remove_test.go Outdated Show resolved Hide resolved

typed/remove_test.go Outdated Show resolved Hide resolved

kwiesmueller mentioned this pull request Dec 9, 2019

[WIP] Apply status wiping kubernetes/kubernetes#86083

Closed

jennybuckley mentioned this pull request Dec 10, 2019

Umbrella Issue for Server Side Apply kubernetes/kubernetes#73723

Closed

kwiesmueller force-pushed the extend-remove branch from 3c49fc4 to 28e231e Compare December 11, 2019 13:19

apelisse reviewed Dec 21, 2019

View reviewed changes

value/value.go Show resolved Hide resolved

apelisse reviewed Dec 21, 2019

View reviewed changes

typed/typed.go Outdated Show resolved Hide resolved

kwiesmueller force-pushed the extend-remove branch from b1a49df to 24c650a Compare December 23, 2019 20:19

kwiesmueller changed the title ~~[WIP] add extended remove method for typed values~~ add extended remove method for typed values Dec 23, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2019

kwiesmueller changed the title ~~add extended remove method for typed values~~ [WIP] add extended remove method for typed values Dec 23, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2019

kwiesmueller force-pushed the extend-remove branch from 968020e to 5151f1a Compare January 4, 2020 18:47

apelisse reviewed Jan 8, 2020

View reviewed changes

typed/remove_test.go Outdated Show resolved Hide resolved

kwiesmueller force-pushed the extend-remove branch 2 times, most recently from 37509fd to bf5b0dc Compare January 15, 2020 23:13

apelisse reviewed Jan 16, 2020

View reviewed changes

typed/remove.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 21, 2020

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 18, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 18, 2020

apelisse reviewed Feb 18, 2020

View reviewed changes

apelisse reviewed Feb 26, 2020

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 3, 2020

apelisse reviewed Mar 6, 2020

View reviewed changes

apelisse reviewed Mar 11, 2020

View reviewed changes

internal/fixture/state.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned jennybuckley Mar 17, 2020

kwiesmueller force-pushed the extend-remove branch from 821eea9 to 0548f89 Compare March 25, 2020 14:52

kwiesmueller changed the title ~~[WIP] add extended remove method for typed values~~ add extended remove method for typed values Mar 25, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2020

kwiesmueller mentioned this pull request Mar 25, 2020

[WIP] Server-Side Apply: Status Wiping/Reset Fields implementation kubernetes/kubernetes#89486

Closed

kwiesmueller force-pushed the extend-remove branch from 0548f89 to fb8f1b2 Compare April 10, 2020 17:07

apelisse reviewed Apr 10, 2020

View reviewed changes

fieldpath/element.go Outdated Show resolved Hide resolved

add extended remove method for TypedValueto support recursive removal…

0856fcb

… of fields

kwiesmueller force-pushed the extend-remove branch from fb8f1b2 to 0856fcb Compare April 10, 2020 17:19

kwiesmueller mentioned this pull request May 2, 2020

Add ignored fields to update #161

Merged

1 task

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 9, 2020

apelisse closed this Jul 9, 2020

add extended remove method for typed values #133

add extended remove method for typed values #133

Uh oh!

Conversation

kwiesmueller commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwiesmueller commented Dec 23, 2019

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwiesmueller Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwiesmueller commented Jan 10, 2020

Uh oh!

kwiesmueller commented Jan 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwiesmueller commented Feb 25, 2020

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwiesmueller Jan 8, 2020 •

edited

Loading