Add ignored fields to update #161
Conversation
k8s-ci-robot
added
sig/api-machinery
|
I'm a little confused why there are all these commits in there, do you need #133? You also have a commit that has already been merged. |
kwiesmueller
force-pushed
the
ignore-fields
branch
4 times, most recently
from
72cf25b to
4efc49f
Compare
kwiesmueller
force-pushed
the
ignore-fields
branch
from
4efc49f to
fd0df01
Compare
kwiesmueller
force-pushed
the
ignore-fields
branch
from
f055e5d to
eccbede
Compare
| } | ||
| if ignored != nil { | ||
| set = set.RecursiveDifference(ignored) | ||
| // TODO: is this correct. If we don't remove from lastSet pruning might remove the fields? |
There was a problem hiding this comment.
After thinking about it again, this might not be required.
Pruning should remove fields from the object if they are not present in newObject and ignored.
Please confirm
There was a problem hiding this comment.
lastSet.Set().RecursiveDifference(ignored)
I think that has to be wrong, we're not editing "VersionedSet" in place, and also RecursiveDifference returns a new object, so that's a no-op right now.
I would assume that the lastSet has been trimmed already (during the operation that set it), but it's possible that the ignored fields have changed since that manager was written, so doing it again seem reasonable.
|
This is really starting to look good, a few things to fix and we'll be good to go! |
|
Daniel, do you want to have a look at that "RecursiveDifference" code? |
fieldpath/set_test.go
Outdated
| name string | ||
| a *Set | ||
| b *Set | ||
| expect *Set |
There was a problem hiding this comment.
Can you add in the expectation for the regular difference? I'm actually not following how this is different?
There was a problem hiding this comment.
I added both differences to the test and added the test i forgot for both.
fieldpath/set.go
Outdated
| // | ||
| // Compared to a regular difference, this recursively removes | ||
| // all children from s | ||
| // that are either children or members in s2 |
There was a problem hiding this comment.
Can we align this comment more with the one for Difference? I'm having a hard time understanding the difference (no pun intended). Feel free to adjust both comments, the existing one isn't amazingly clear.
There was a problem hiding this comment.
Can you check again?
The difference is, that a Difference only respects fields that are directly specified.
So to remove a.b.c and a.b.d from s, both need to be in s2.
But RecursiveDifference behaves more like deleting a folder that contains folders. It's enough to specify a in s2 and it's members are deleted recursively, or in the case of RecursiveDifference not added to the result.
I tried a few different explanations, but none was really good. I'm open for suggestions :-/
|
Thanks, it's much clearer now. /approve (I only looked at the tests & documentation, maybe @apelisse or @jennybuckley can review the rest and lgtm) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kwiesmueller, lavalamp 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I had a look at the code, thanks! |
k8s-ci-robot
added
the
lgtm
5 participants
The fieldset provided to the update/apply will be excluded from any managedFields changes, but still be reflected in the actual update.
/sig api-machinery
/wg api-expression
/cc @apelisse