typed: Update compare algorithm to handle duplicates #251
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
typed/compare.go
Outdated
| continue | ||
|
|
||
| if v, found := lValues.Get(pe); found { | ||
| list := v.Unstructured().([]value.Value) |
There was a problem hiding this comment.
Would it be preferable to avoid unstructured conversion for the comparison? This kinda makes me sad
There was a problem hiding this comment.
Yeah I dont understand how this line works. Shouldn't Unstructured return a []interface{}?
There was a problem hiding this comment.
Yeah, it's because I'm abusing the PathElementValueMap here. I don't insert a valid value in there, I cast a []value.Value into a value and then retrieve it, and it just works. I don't know if we want to introduce generics code in here just yet, so I can't really make the map take whatever type of parameter type I want.
| if len(rList) != 0 { | ||
| rValue = rList[0] | ||
| } | ||
| errs = append(errs, w.compareListItem(t, pe, lValue, rValue)...) |
There was a problem hiding this comment.
If I'm reading this correctly, this is the "no duplicates" case and should be semantically identical to the old code?
apelisse
force-pushed
the
add-compare-duplicates
branch
from
7d04114 to
8f742cc
Compare
|
OK, I've added comments to explain what happens, hopefully that helps. |
typed/compare.go
Outdated
| continue | ||
|
|
||
| if v, found := lValues.Get(pe); found { | ||
| list := v.Unstructured().([]value.Value) |
There was a problem hiding this comment.
Yeah I dont understand how this line works. Shouldn't Unstructured return a []interface{}?
apelisse
force-pushed
the
add-compare-duplicates
branch
from
8f742cc to
a1f0e95
Compare
|
OK I've added a new commit to create a new map of PEs to |
|
/lgtm in case you wanted to change anything else |
k8s-ci-robot
added
do-not-merge/hold
|
/hold I'm good thanks! |
|
I meant |
k8s-ci-robot
removed
the
do-not-merge/hold
Same strategy as #249, still trying to address #234. Algorithm goes as follow:
Rewording:
In compare, treat duplicate keys in associative lists/sets as a separate atomic list, marked by owning just the duplicated key.
/assign @alexzielenski
/cc @jpbetz