-
Notifications
You must be signed in to change notification settings - Fork 39.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support setElementOrder #45980
support setElementOrder #45980
Conversation
/assign @apelisse @pwittrock |
/unassign @fgrzadkowski @timstclair |
staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file):
I wish we could simplify the way this method is called:
Comments from Reviewable |
Review status: 0 of 16 files reviewed at latest revision, 1 unresolved discussion. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file):
What does it mean?
That's reasonable. Comments from Reviewable |
staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file):
When you do:
So it could look like this:
Comments from Reviewable |
Review status: 0 of 16 files reviewed at latest revision, 1 unresolved discussion. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file): Previously, apelisse (Antoine Pelisse) wrote…
Got your point. Comments from Reviewable |
Reviewed 14 of 16 files at r1. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 515 at r1 (raw file):
I wonder if staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 527 at r1 (raw file):
It also makes me sad that we have to special case on staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1007 at r1 (raw file):
This function is doing two different things based on the presence of Comments from Reviewable |
Review status: 14 of 16 files reviewed at latest revision, 5 unresolved discussions. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 990 at r2 (raw file):
That's not really what I meant. I think you should have two different functions, and the caller of this function should decide which one to call. Comments from Reviewable |
@apelisse Updated. |
LGTM. Can someone else please have a look? Thanks @mengqiy |
@liggitt Can you please review this PR to make sure nothing goes wrong? The implementation is a little different from the one in kubernetes/community#537 (comment). |
Automatic merge from submit-queue improve type assertion error Per discussion #45980 (comment). ```release-note NONE ```
Review status: 7 of 12 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 332 at r3 (raw file): Previously, alexandercampbell (Alexander Campbell) wrote…
That works too. Worth noting that they are not equivalent or interchangeable, as staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 829 at r4 (raw file): Previously, mengqiy (Mengqi Yu) wrote…
I am glad this is tested, but are these for testing the element order specifically, or just broader coverage? staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 604 at r5 (raw file):
'2' should not be present right? staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1087 at r5 (raw file):
'be present' staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1116 at r5 (raw file):
This is a good test case +1 staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1576 at r5 (raw file):
I think the description is wrong. The item is not duplicate right? staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 2708 at r5 (raw file):
Awesome. Thank for checking this edge case, I would not have thought of it staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 2725 at r5 (raw file):
Use different values or invert the order of the inner list to make sure it the correct directive is used staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 3603 at r5 (raw file):
Cool test case staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 5953 at r5 (raw file):
Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 20 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 337 at r4 (raw file):
What is the Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 358 at r3 (raw file): Previously, mengqiy (Mengqi Yu) wrote…
document the arguments in comments. what is left vs right? update the comment with how this preserves order amongst the slices - which has higher precedence? Are they interleaved or all of one before the other? staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file):
What are the arguments - add comments staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file):
instead of calling this staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 357 at r4 (raw file):
how is staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 368 at r4 (raw file):
are the lists guaranteed to have non-overlapping elements - add a comment if so staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 372 at r4 (raw file):
add comments explaining what this logic is supposed to accomplish and how it works staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 398 at r4 (raw file):
add comment explaining this block. it isn't obvious what it is for Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 27 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 322 at r4 (raw file):
comment explaining what this function does Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 937 at r4 (raw file):
heavily comment this code since it isn't obvious what it is doing staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 938 at r4 (raw file):
Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 30 unresolved discussions. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 515 at r1 (raw file): Previously, apelisse (Antoine Pelisse) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 527 at r1 (raw file): Previously, apelisse (Antoine Pelisse) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1007 at r1 (raw file): Previously, apelisse (Antoine Pelisse) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 990 at r2 (raw file): Previously, apelisse (Antoine Pelisse) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 60 at r3 (raw file): Previously, alexandercampbell (Alexander Campbell) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 332 at r3 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 358 at r3 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 376 at r3 (raw file): Previously, alexandercampbell (Alexander Campbell) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 395 at r3 (raw file): Previously, alexandercampbell (Alexander Campbell) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 577 at r3 (raw file): Previously, alexandercampbell (Alexander Campbell) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 322 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 337 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 357 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 368 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
In current implementation, they are guaranteed to be non-overlapping. But this function doesn't require they to be non-overlapping. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 372 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 398 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 937 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 938 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 829 at r4 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
The test cases below the comment are for testing the element order specifically. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 604 at r5 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
This test case is checking the error of mismatching order should be catch. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1087 at r5 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1576 at r5 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Updated. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 5953 at r5 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. Comments from Reviewable |
/approve |
/lgtm |
Review status: 7 of 12 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 2725 at r5 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. Comments from Reviewable |
Review status: 7 of 12 files reviewed at latest revision, 53 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 509 at r6 (raw file):
normalizeSliceOrder staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 594 at r6 (raw file):
staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 601 at r6 (raw file):
if (!mergeKeyValueEqual(foo, bar, mergeKey) {return true} staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 976 at r6 (raw file):
Add comment that the second check is is a sanity check, and should always be true if the first is true staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1067 at r6 (raw file):
Add comment explaining this deletes the element staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1068 at r6 (raw file):
staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1074 at r6 (raw file):
setElementOrderInPatch := patchV staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1080 at r6 (raw file):
// Copies directive from first patch to second patch and checks they are equal staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1084 at r6 (raw file):
setElementOrderListInPatch staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1094 at r6 (raw file):
// Function to take 2 maps + directive, returns typeCasted field values staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1096 at r6 (raw file):
originalFieldValue, patchFieldValue staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1105 at r6 (raw file):
// Trim the setElementOrderDirectivePrefix to get the key of the list field in staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1124 at r6 (raw file):
Do validation here for setElementOrderListInPatch against the field value: staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1129 at r6 (raw file):
move this next to the code that uses it staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1134 at r6 (raw file):
// getMergedSliceValue - take original + patch values + found in original + patch, returns merged staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1135 at r6 (raw file):
// no change to list contents staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1137 at r6 (raw file):
// list was added staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1140 at r6 (raw file):
// Check for consistency between the element order list and the field it applies to staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1141 at r6 (raw file):
move above, see other comment staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1155 at r6 (raw file):
// If we are merging the patch into the server object, identify elements missing from the ordering staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1158 at r6 (raw file):
leave comment why primitives are different than maps staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1175 at r6 (raw file):
check if we only need to do this for mergeOptions.MergeParallelList, I think when we are merging patches together, they should already be in the correct order staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1187 at r6 (raw file):
// partitionItemsByPresentInList partitions staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1256 at r6 (raw file):
Add comment that this will delete the merged elements from patch so they will not be reprocessed Comments from Reviewable |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
Review status: 7 of 12 files reviewed at latest revision, 53 unresolved discussions, some commit checks failed. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 509 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 594 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 601 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 976 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1067 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1068 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1074 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1080 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1084 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1096 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1105 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1124 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1129 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
validation code need merge key. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1135 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1137 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1140 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1141 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1158 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1175 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Yes, we need this when merging 2 patches since one may contains deleting a field from a map; the other may be add a field in the map. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1187 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1256 at r6 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done. Comments from Reviewable |
Tracked the unresolved refactoring comments in #46543, will address them later. |
@k8s-bot pull-kubernetes-unit test this |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengqiy, pwittrock The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
Automatic merge from submit-queue improve type assertion error Per discussion kubernetes/kubernetes#45980 (comment). ```release-note NONE ``` Kubernetes-commit: 4d6ef25f6457d2019e57f681d2d627d4f32e3705
Implement proposal.
Fixes #40373