Fix remove structures #184
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
|
I just did a pass on the WG API Expression: Unsetting fields to make sure we're not contradicting anything we decided on there. I think we're OK. |
By "null" we really just mean "empty struct" here, right? In WG API Expression: Unsetting fields I remember that we were deliberately trying to avoid the distinction between |
Can we say more about the motivation for this in the PR description? This seems to complicate how the ownership transfer mechanism of Server Side Apply behaves-- updater ownership is now handled differently. Or were we already handling it differently and I just wasn't aware? |
fieldpath/set.go
Outdated
| } | ||
| } | ||
|
|
||
| // NestedSet returns a Set that contains all the fields in s, as well as |
There was a problem hiding this comment.
NestedSet is not a good name, I think the name should include something about ensuring all children are also members. RecursivelyCompletelyCoveredSet or something like that? EnsureAllChildrenAreMembers?
merge/ignore_test.go
Outdated
| mapOfMapsRecursive: | ||
| `, | ||
| Manager: "apply-one", | ||
| Object: ``, |
There was a problem hiding this comment.
hm I can see why you changed this test case but I think we need to know what the output is if clients change nothing?
There was a problem hiding this comment.
I guess my ask is, leave all the inputs the same and change the outputs in these tests. And copy the cases and modify them if you want to test different inputs.
There was a problem hiding this comment.
I've made the change you requested. I understand that as a reviewer you want to see how the behavior changed, but I think the test is better the way it was written before.
| v2.value = val | ||
| errs = append(errs, v2.toFieldSet()...) | ||
| if _, ok := t.FindField(key); !ok { | ||
| if val.IsNull() || (val.IsMap() && val.AsMap().Length() == 0) { |
There was a problem hiding this comment.
zero length maps but not zero length lists? That's surprising?
There was a problem hiding this comment.
Actually, this is specific to struct fields with no values, it's not relevant for lists.
apelisse
force-pushed
the
fix-remove-structures
branch
from
e373eba to
e71bab7
Compare
| Object: typed.YAMLObject(` | ||
| struct: | ||
| name: a | ||
| complexField_%s: null |
There was a problem hiding this comment.
is this case still tested?
There was a problem hiding this comment.
Joe had to write the test with the "dangling" struct only because of this bug that I'm fixing. This is the bug fix :-)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, 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 |
4 participants
Fixes #171
This fixes a bug and slightly changes an existing behavior.
Struct that have a null value will now be owned, i.e.
will now be owned by whoever applies that (currently, these can't be owned, which means we may accidentally drop the empty struct).
Also, removing a struct will now remove all the items that are not owned by someone else in it (including things that are owned by an updater).