kubernetes-sigs / structured-merge-diff

Test cases and implementation for "server-side apply"
Apache License 2.0
97 stars 58 forks source link

merge: Allow duplicate keys in lhs #254

Closed apelisse closed 11 months ago

apelisse commented 11 months ago

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.

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

k8s-ci-robot commented 11 months ago

[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](https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/OWNERS)~~ [apelisse] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
liggitt commented 11 months ago

/hold to prevent accidental merge until review is done

looks like there are relevant unit test failures

apelisse commented 11 months ago

looks like there are relevant unit test failures

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. Or I could try to fix the union code but that doesn't sound like a useful effort.

liggitt commented 11 months ago

Yeah, the previous PR somewhat broke the union code that is technically not used

wait... those tests passed on that PR though, right?

liggitt commented 11 months ago

oh, these are new tests added in this PR

apelisse commented 11 months ago

Tests are passing now, ready to merge, PTAL!

liggitt commented 11 months ago

/hold cancel /lgtm