merge: Allow duplicate keys in lhs #254
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
/hold to prevent accidental merge until review is done looks like there are relevant unit test failures |
k8s-ci-robot
added
the
do-not-merge/hold
| @@ -179,7 +179,7 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp | |||
| ignored = fieldpath.NewSet() | |||
| } | |||
| managers[manager] = fieldpath.NewVersionedSet( | |||
| managers[manager].Set().Union(compare.Modified).Union(compare.Added).Difference(compare.Removed).RecursiveDifference(ignored), | |||
| managers[manager].Set().Difference(compare.Removed).Union(compare.Modified).Union(compare.Added).RecursiveDifference(ignored), | |||
There was a problem hiding this comment.
is this the only functional line changing?
I'm trying to figure out why the order change matters
There was a problem hiding this comment.
When we go from duplicate to non-duplicate and vice versa, we add/remove the managedfields for duplicates and then remove/add the managedfields for the non-duplicated form. That does an add AND a remove, possibly of the same key. Because of how the line was written before, we would add the add and then remove the remove, which doesn't work for fields that are in both.
There was a problem hiding this comment.
is this the only functional line changing?
Yes
Yeah, the previous PR somewhat broke the union code that is technically not used. Since we might want to backport that change, I don't want to break the API. Not sure how to proceed. I could remove the internal code and just panic while keeping the actual API. |
wait... those tests passed on that PR though, right? |
|
oh, these are new tests added in this PR |
This not only affects merge but also `Compare` since they use the same algorithm/code. Duplicates fields in a set/associative-list will now be treated as an atomic entity within that list, and will be entirely owned by the person who made them duplicates or who changed one of the duplicates.
apelisse
force-pushed
the
allow-duplicates-in-associative-lists
branch
from
4c3addd to
29babbc
Compare
|
Tests are passing now, ready to merge, PTAL! |
|
/hold cancel |
k8s-ci-robot
added
lgtm
This not only affects merge but also
Comparesince they use the same algorithm/code.Duplicates fields in a set/associative-list will now be treated as an atomic entity within that list, and will be entirely owned by the person who made them duplicates or who changed one of the duplicates.
That's the final fix for #234. This comes with extensive testing of all the scenarios I could think of. None of the existing behavior has changed.
/assign @liggitt @alexzielenski