typed: Allow duplicates when we merge #253
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
| errs = append(errs, errorf("duplicate entries for key %v", pe.String())...) | ||
| continue | ||
| if _, found := observed.Get(pe); !found { | ||
| observed.Insert(pe, child) |
There was a problem hiding this comment.
Is this existence check necessary? Is observed a set?
There was a problem hiding this comment.
observed is a map from pathelement to the actual value.
the value that ends-up being in the PE is the one we're going to use to merge I think, so if we were to override here, I think we would end-up merging with the last item in the list rather than the first. But since we've decided we might not want to do that, I will probably deal with that differently.
| mergeOut, errs := w.mergeListItem(t, pe, lChild, nil) | ||
| errs = append(errs, errs...) | ||
| if mergeOut != nil { | ||
| out = append(out, *mergeOut) | ||
| } | ||
| lI++ | ||
| continue | ||
| } else if _, ok := mergedRHS.Get(pe); ok { | ||
| // we've already merged it with RHS, we don't want to duplicate it, skip it. |
There was a problem hiding this comment.
I'm not sure why this case is necessary. It's hard to track the invariants of observedRHS vs mergedRHS without going very deep into the code.
typed/merge.go
Outdated
| @@ -180,14 +180,18 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs value.List) (err | |||
| } | |||
| out := make([]interface{}, 0, outLen) | |||
|
|
|||
| rhsOrder, observedRHS, rhsErrs := w.indexListPathElements(t, rhs) | |||
| rhsPEs, observedRHS, rhsErrs := w.indexListPathElements(t, rhs) | |||
There was a problem hiding this comment.
Are the PE's no longer ordered?
There was a problem hiding this comment.
They are still, but "order" didn't really tell me what they were. I could revert or make it rhsOrderedPEs or something.
apelisse
force-pushed
the
merge-duplicates
branch
from
c2d986f
to
7c83c0a
Compare
The goal is to ignore duplicates if they are not included in the partial object, and to replace all of the occurences if they are in the partial object.
apelisse
force-pushed
the
merge-duplicates
branch
from
7c83c0a
to
4daa91c
Compare
|
PTAL, I think I've addressed all the commits. Also split the rename of rhsOrder and lhsOrder in a seperate commit to keep the meat of the change isolated. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Thanks, open the k/k PR to update master and ping me for review. I suspect we'll want to backport this, not sure if that means picking the fix to other smd branches or not. |
The goal is to ignore duplicates if they are not included in the partial object, and to replace all of the occurences if they are in the partial object.
/assign @liggitt
/cc @alexzielenski