support setElementOrder #45980

mengqiy · 2017-05-17T18:36:39Z

kubectl edit and kubectl apply will keep the ordering of elements in merged lists

mengqiy · 2017-05-17T18:37:13Z

/assign @apelisse @pwittrock

mengqiy · 2017-05-17T19:39:54Z

/unassign @fgrzadkowski @timstclair

k8s-reviewable · 2017-05-17T21:51:51Z

This change is

apelisse · 2017-05-18T19:41:22Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file):

			if !ok {
				t := reflect.TypeOf(item)
				return mergepatch.ErrBadArgType("map[string]interface{}", t.Kind().String())

I wish we could simplify the way this method is called:

First it's repeating the type, which is error-prone.
Second, maybe it would be simpler if it just received item, and then ran the reflect.TypeOf(item).Kind().String() in the function directly?

Comments from Reviewable

mengqiy · 2017-05-18T19:52:22Z

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):

it's repeating the type

What does it mean?

maybe it would be simpler if it just received item, and then ran the reflect.TypeOf(item).Kind().String() in the function directly?

That's reasonable.

Comments from Reviewable

apelisse · 2017-05-18T20:01:23Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 330 at r1 (raw file):

What does it mean?

When you do:

m, ok := item.(map[string]interface{})
if !ok {
   t := reflect.TypeOf(item)
   return mergepatch.ErrBadArgType("map[string]interface{}", t.Kind().String())

map[string]interface{} is repeated, once for casting, once for the error. Meaning that someone copy-pasting this pattern somewhere else might forget to update the latter. Interestingly, you could use m for that.

So it could look like this:

m, ok := item.(map[string]interface{})
if !ok {
    return mergepatch.ErrBadArgTpe(m, item)
}

Comments from Reviewable

mengqiy · 2017-05-18T20:54:14Z

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…

What does it mean?

When you do:
m, ok := item.(map[string]interface{})
if !ok {
   t := reflect.TypeOf(item)
   return mergepatch.ErrBadArgType("map[string]interface{}", t.Kind().String())
map[string]interface{} is repeated, once for casting, once for the error. Meaning that someone copy-pasting this pattern somewhere else might forget to update the latter. Interestingly, you could use m for that.

So it could look like this:
m, ok := item.(map[string]interface{})
if !ok {
    return mergepatch.ErrBadArgTpe(m, item)
}

Got your point.
I'm sending a separate PR for it, since I don't want to introduce unrelated change to this PR. (it already big)
I will let you review it.

Comments from Reviewable

apelisse · 2017-05-18T21:10:46Z

Reviewed 14 of 16 files at r1.
Review status: 14 of 16 files reviewed at latest revision, 4 unresolved discussions.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 515 at r1 (raw file):

	kind := elementType.Kind()
	if diffOptions.SetElementOrder {
		setOrderList, err = genSetOrderList(modified, mergeKey, kind)

I wonder if genSetOrderList should be removed and its content moved into the following switch-statement.
genSetOrderList only seem to make sense for a subset of the cases anyway.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 527 at r1 (raw file):

	}
	patchList, err = sortSliceByOneOrder(patchList, modified, mergeKey, kind)
	if kind == reflect.Map {

It also makes me sad that we have to special case on Map two lines after the switch :-/

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1007 at r1 (raw file):

	patch := make([]interface{}, 0, len(original))
	serverOnly := make([]interface{}, 0, len(original))
	if len(mergeKey) == 0 {

This function is doing two different things based on the presence of mergeKey or not. Maybe it should be split into two different functions (one that takes mergeKey, if it's a list of primitives, and one if it's a list of maps).

Comments from Reviewable

apelisse · 2017-05-18T22:32:32Z

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):

// extractServerOnlyItems return patchItems and server-only items in their original relative order
func extractServerOnlyItems(original, setElementOrderList []interface{}, mergeKey string) ([]interface{}, []interface{}, error) {

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

mengqiy · 2017-05-18T23:28:29Z

@apelisse Updated.

apelisse · 2017-05-19T19:22:26Z

LGTM. Can someone else please have a look?

Thanks @mengqiy

mengqiy · 2017-05-22T17:54:03Z

@liggitt Can you please review this PR to make sure nothing goes wrong?
/assign @liggitt

The implementation is a little different from the one in kubernetes/community#537 (comment).
Because of how sort.SliceStable() implemented, using it may not work in some case.
e.g. https://play.golang.org/p/jyGQssCFdK we are expect [4,1,2,3,9] or [4,1,2,9,3].
So I first sort the items in the directive and server-only items and then merge them.

Automatic merge from submit-queue improve type assertion error Per discussion #45980 (comment). ```release-note NONE ```

pwittrock · 2017-05-30T17:16:23Z

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…

Doesn't really matter, but you can just do = instead of :=

That works too. Worth noting that they are not equivalent or interchangeable, as := will declare a new ok in a new scope, so the original ok will not have the updated value outside of the if block scope. In this case it doesn't matter.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 829 at r4 (raw file):

Previously, mengqiy (Mengqi Yu) wrote…

There is a comment in the code
// test case for setElementOrder
All the test cases below it are new test cases.
The others are move from regular test case to raw test cases since we need to add directive.

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):

$setElementOrder/mergingList:
  - name: 1
  - name: 2

'2' should not be present right?

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1087 at r5 (raw file):

      retainKeysMap:
        name: foo
  - description: retainKeys map with no change should not present

'be present'

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1116 at r5 (raw file):

var strategicMergePatchRawTestCases = []StrategicMergePatchRawTestCase{
	{
		Description: "delete items in lists of scalars",

This is a good test case +1

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1576 at r5 (raw file):

	},
	{
		Description: "add duplicate item in merging list",

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):

	},
	{
		Description: "add field to map in merging list nested in merging list with deletion conflict",

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):

mergingList:
  - $setElementOrder/mergingList:
      - name: 1

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):

	},
	{
		Description: "set elements order in a list with server-only items 2",

Cool test case

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 5953 at r5 (raw file):

			return
		}
		t.Errorf("expected error should contains:\n%s\nin test case: %s\nbut got:\n%s\n", expectedError, description, err)

expected error should contain: - no 's'

Comments from Reviewable

pwittrock · 2017-05-30T17:19:38Z

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):

}

// sortSliceByMultipleOrders sort `patch` list by patchOrder and

What is the patch list and patchOrder - please clarify in the comments. Also what is the serverOnly list and serverOrder? How are the lists merged?

Comments from Reviewable

pwittrock · 2017-05-30T17:30:12Z

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…

The relative order of two items are determined by the following order:

relative order in the $setElementOrder if both items are present

else relative order in the patch if both items are present

else relative order in the server-side list if both items are present

else append to the end

We are tying to sort the list according to this proposal.
But there may be ordering conflict. e.g.
live list: ["a", "x", "b", "y", "c", "z"]
patch list: ["c", "b", "a"]
There is no way to make a list satisfy:

a comes before x

x comes before b

b comes before a

In this PR's implementation, we can guarantee the relative order in the patch list and the relative order between server-only items.
We have 2 sorted list: sorted patch list and sorted server-only items list.
We are trying to merge these 2 lists.
Please check sortSliceByMultipleOrders.

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):

// Then it merges the 2 sorted lists by serverOrder with best effort.
// It guarantee the relative order in the patch list and in the serverOnly list is kept.
func sortSliceByMultipleOrders(patch, serverOnly, patchOrder, serverOrder []interface{}, mergeKey string, kind reflect.Kind) ([]interface{}, error) {

What are the arguments - add comments

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file):

// Then it merges the 2 sorted lists by serverOrder with best effort.
// It guarantee the relative order in the patch list and in the serverOnly list is kept.
func sortSliceByMultipleOrders(patch, serverOnly, patchOrder, serverOrder []interface{}, mergeKey string, kind reflect.Kind) ([]interface{}, error) {

instead of calling this sortSliceByMultipleOrders how about normalizeElementOrder

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 357 at r4 (raw file):

// mergeSortedSlice merges the 2 sorted lists by serverOrder with best effort.
func mergeSortedSlice(left, right, serverOrder []interface{}, mergeKey string, kind reflect.Kind) []interface{} {
	// returns if l is less than r, and if both have been found.

how is less than r defined? - add comment explaining what this function is support to return

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 368 at r4 (raw file):

	}

	size := len(left) + len(right)

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):

	s := make([]interface{}, size, size)

	for k := 0; k < size; k++ {

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):

	var getValFn func(interface{}) interface{}
	switch kind {
	case reflect.Map:

add comment explaining this block. it isn't obvious what it is for

Comments from Reviewable

pwittrock · 2017-05-30T17:36:25Z

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):

}

func validateMergeKeyInLists(mergeKey string, lists ...[]interface{}) error {

comment explaining what this function does

Comments from Reviewable

pwittrock · 2017-05-30T17:37:44Z

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):

// Ref: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/preserve-order-in-strategic-merge-patch.md
func processSetElementOrderList(original, patch map[string]interface{}, t reflect.Type, mergeOptions MergeOptions) error {
	for key, patchV := range patch {

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):

// Do nothing if there is no ordering directive

Comments from Reviewable

mengqiy · 2017-05-30T23:07:12Z

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…

I wonder if genSetOrderList should be removed and its content moved into the following switch-statement.
genSetOrderList only seem to make sense for a subset of the cases anyway.

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 527 at r1 (raw file):

Previously, apelisse (Antoine Pelisse) wrote…

It also makes me sad that we have to special case on Map two lines after the switch :-/

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1007 at r1 (raw file):

Previously, apelisse (Antoine Pelisse) wrote…

This function is doing two different things based on the presence of mergeKey or not. Maybe it should be split into two different functions (one that takes mergeKey, if it's a list of primitives, and one if it's a list of maps).

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 990 at r2 (raw file):

Previously, apelisse (Antoine Pelisse) wrote…

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.

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 60 at r3 (raw file):

Previously, alexandercampbell (Alexander Campbell) wrote…

"controls if generating" --> "determines whether we generate"

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 332 at r3 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

That works too. Worth noting that they are not equivalent or interchangeable, as := will declare a new ok in a new scope, so the original ok will not have the updated value outside of the if block scope. In this case it doesn't matter.

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 358 at r3 (raw file):

Previously, pwittrock (Phillip Wittrock) 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?

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 376 at r3 (raw file):

Previously, alexandercampbell (Alexander Campbell) wrote…

I don't like the "off-by-one" going on here. Why not just do this?
if i >= len(left) && j < len(right) {

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 395 at r3 (raw file):

Previously, alexandercampbell (Alexander Campbell) wrote…

This can be simpler:

if foundBoth && less {
    s[k] = left[i]
    i++
} else {
    s[k] = right[j]
    j++
}

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 577 at r3 (raw file):

Previously, alexandercampbell (Alexander Campbell) wrote…

Ok, sure, that's a good justification. I'm partial to unix convention :)
The spelling of the verb in English is definitely uniquify but I can see how "uniqify" as a word based on the tool "uniq" makes sense.
No need to follow up on this comment @mengqiy -- I think the discussion is resolved. Thanks for your input @apelisse

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 322 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

comment explaining what this function does

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 337 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

What is the patch list and patchOrder - please clarify in the comments. Also what is the serverOnly list and serverOrder? How are the lists merged?

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

What are the arguments - add comments

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 341 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

instead of calling this sortSliceByMultipleOrders how about normalizeElementOrder

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 357 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

how is less than r defined? - add comment explaining what this function is support to return

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 368 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

are the lists guaranteed to have non-overlapping elements - add a comment if so

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…

add comments explaining what this logic is supposed to accomplish and how it works

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 398 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

add comment explaining this block. it isn't obvious what it is for

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 937 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

heavily comment this code since it isn't obvious what it is doing

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 938 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// Do nothing if there is no ordering directive

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 829 at r4 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

I am glad this is tested, but are these for testing the element order specifically, or just broader coverage?

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…

'2' should not be present right?

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…

'be present'

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 1576 at r5 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

I think the description is wrong. The item is not duplicate right?

Updated.
It was trying to say adding an item that already exists in current object.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go, line 5953 at r5 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

expected error should contain: - no 's'

Done.

Comments from Reviewable

pwittrock · 2017-05-31T01:41:25Z

/approve

pwittrock · 2017-05-31T01:41:31Z

/lgtm

mengqiy · 2017-05-31T01:53:09Z

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…

Use different values or invert the order of the inner list to make sure it the correct directive is used

Done.

Comments from Reviewable

pwittrock · 2017-05-31T02:24:32Z

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):

// sortSliceByOneOrder sort `toSort` list by `order`
func sortSliceByOneOrder(toSort, order []interface{}, mergeKey string, kind reflect.Kind) ([]interface{}, error) {

normalizeSliceOrder

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 594 at r6 (raw file):

// orderChangedListOfMaps checks if the order in a list has changed
func orderChangedListOfMaps(original, modified []interface{}, mergeKey string) (bool, error) {

IsOrderSame

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 601 at r6 (raw file):

		originalItem := original[i]
		typedOriginalItem, ok := originalItem.(map[string]interface{})
		if !ok {

if (!mergeKeyValueEqual(foo, bar, mergeKey) {return true}

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 976 at r6 (raw file):

		setOrderIndex++
	}
	if patchIndex < len(nonDeleteList) && setOrderIndex >= len(typedSetOrderList) {

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):

// Then, sort them by the relative order in setElementOrder, patch list and live list.
// The precedence is $setElementOrder > order in patch list > order in live list.
// Ref: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/preserve-order-in-strategic-merge-patch.md

Add comment explaining this deletes the element

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1068 at r6 (raw file):

// The precedence is $setElementOrder > order in patch list > order in live list.
// Ref: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/preserve-order-in-strategic-merge-patch.md
func processSetElementOrderList(original, patch map[string]interface{}, t reflect.Type, mergeOptions MergeOptions) error {

mergePatchIntoOriginal

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1074 at r6 (raw file):

patchV

setElementOrderInPatch := patchV

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1080 at r6 (raw file):

		// - $setElementOrder exists only in patch, move it to original.
		// - $setElementOrder exists only in original, do nothing.
		if !mergeOptions.MergeParallelList {

// 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):

			if ok {
				// check if the setElementOrder list in original and the one in patch matches
				if !reflect.DeepEqual(setElementOrderListInOriginal, patchV) {

setElementOrderListInPatch

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1094 at r6 (raw file):

		delete(patch, key)

		var (

// 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):

		var (
			ok                                        bool
			typedOriginalList, typedPatchList, merged []interface{}

originalFieldValue, patchFieldValue

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1105 at r6 (raw file):

// Trim the setElementOrderDirectivePrefix to get the original key.

// Trim the setElementOrderDirectivePrefix to get the key of the list field in original.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1124 at r6 (raw file):

				return mergepatch.ErrBadArgType(typedPatchList, patchList)
			}
		}

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):

			return err
		}
		_, patchStrategy, err = extractRetainKeysPatchStrategy(patchStrategies)

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):

		}

		switch {

// 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):

		switch {
		case foundOriginal && !foundPatch:

// no change to list contents

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1137 at r6 (raw file):

		case foundOriginal && !foundPatch:
			merged = typedOriginalList
		case !foundOriginal && foundPatch:

// list was added

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1140 at r6 (raw file):

			merged = typedPatchList
		case foundOriginal && foundPatch:
			// validate patch list

// 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):

		case foundOriginal && foundPatch:
			// validate patch list
			err := validatePatchWithSetOrderList(typedPatchList, typedSetElementOrderList, mergeKey)

move above, see other comment

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1155 at r6 (raw file):

		}

		// Split all items into patch items and server-only items and then enforce the order.

// If we are merging the patch into the server object, identify elements missing from the ordering
// so we can order them separately
if (mergeOptions.MergeParallelList ) {
merged, err := applyElementOrder(merged, typeSetElementtOrderList)
} else {
original[originalKey] = merged
}

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1158 at r6 (raw file):

		var patchItems, serverOnlyItems []interface{}
		if len(mergeKey) == 0 {
			patchItems, serverOnlyItems = extractServerOnlyItemsFromListOfPrimitives(merged, typedSetElementOrderList)

leave comment why primitives are different than maps

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1175 at r6 (raw file):

		// typedSetElementOrderList contains all the relative order in typedPatchList,
		// so don't need to use typedPatchList
		both, err := normalizeElementOrder(patchItems, serverOnlyItems, typedSetElementOrderList, typedOriginalList, mergeKey, kind)

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):

// extractServerOnlyItemsFromListOfPrimitives return patchItems and server-only items in their original relative order
func extractServerOnlyItemsFromListOfPrimitives(original, setElementOrderList []interface{}) ([]interface{}, []interface{}) {

// partitionItemsByPresentInList partitions elements into 2 slices, the first containing items present in partionBy
partitionElementsByPresentInList()

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1256 at r6 (raw file):

	// When not merging the directive, it will make sure $setElementOrder list exist only in original.
	// When merging the directive, it will process $setElementOrder and its patch list together.
	err = processSetElementOrderList(original, patch, t, mergeOptions)

Add comment that this will delete the merged elements from patch so they will not be reprocessed

Comments from Reviewable

pwittrock · 2017-05-31T21:10:49Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

mengqiy · 2017-05-31T22:02:32Z

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…

normalizeSliceOrder

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 594 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

IsOrderSame

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 601 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

if (!mergeKeyValueEqual(foo, bar, mergeKey) {return true}

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 976 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comment that the second check is is a sanity check, and should always be true if the first is true

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1067 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comment explaining this deletes the element

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1068 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

mergePatchIntoOriginal

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1074 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

patchV

setElementOrderInPatch := patchV

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1080 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// Copies directive from first patch to second patch and checks they are equal

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1084 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

setElementOrderListInPatch

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1096 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

originalFieldValue, patchFieldValue

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1105 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// Trim the setElementOrderDirectivePrefix to get the original key.

// Trim the setElementOrderDirectivePrefix to get the key of the list field in original.

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1124 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Do validation here for setElementOrderListInPatch against the field value:

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1129 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

move this next to the code that uses it

validation code need merge key.
So we can't move this down after moving validation up.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1135 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// no change to list contents

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1137 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// list was added

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1140 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// Check for consistency between the element order list and the field it applies to

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1141 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

move above, see other comment

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1158 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

leave comment why primitives are different than maps

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1175 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

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

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.
After merging these 2 patches, we want to normalize the order.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1187 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

// partitionItemsByPresentInList partitions elements into 2 slices, the first containing items present in partionBy
partitionElementsByPresentInList()

Done.

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go, line 1256 at r6 (raw file):

Previously, pwittrock (Phillip Wittrock) wrote…

Add comment that this will delete the merged elements from patch so they will not be reprocessed

Done.

Comments from Reviewable

mengqiy · 2017-05-31T22:21:18Z

Tracked the unresolved refactoring comments in #46543, will address them later.

mengqiy · 2017-05-31T23:00:09Z

@k8s-bot pull-kubernetes-unit test this

mengqiy · 2017-06-01T00:01:39Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

pwittrock · 2017-06-01T14:36:39Z

/lgtm

k8s-github-robot · 2017-06-01T14:42:22Z

[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:

~~pkg/kubectl/OWNERS~~ [pwittrock]
~~staging/src/k8s.io/apimachinery/pkg/util/mergepatch/OWNERS~~ [pwittrock]
~~staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/OWNERS~~ [pwittrock]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-01T16:12:13Z

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

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 17, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 17, 2017

k8s-github-robot added the release-note-label-needed label May 17, 2017

k8s-github-robot assigned fgrzadkowski and timstclair May 17, 2017

k8s-ci-robot assigned apelisse and pwittrock May 17, 2017

mengqiy force-pushed the setElementOrder branch from 67b07f2 to 7156b31 Compare May 17, 2017 19:38

k8s-ci-robot unassigned timstclair and fgrzadkowski May 17, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 17, 2017

mengqiy mentioned this pull request May 18, 2017

improve type assertion error #46057

Merged

mengqiy force-pushed the setElementOrder branch from dc01d9e to cf2a60d Compare May 18, 2017 23:27

apelisse removed their assignment May 19, 2017

k8s-ci-robot assigned liggitt May 22, 2017

k8s-github-robot pushed a commit that referenced this pull request May 22, 2017

Merge pull request #46057 from mengqiy/improvePatchTypeError

4d6ef25

Automatic merge from submit-queue improve type assertion error Per discussion #45980 (comment). ```release-note NONE ```

pwittrock assigned alexandercampbell May 23, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

mengqiy force-pushed the setElementOrder branch from 68195a8 to 65b1632 Compare May 31, 2017 22:01

support setElementOrder

c3481b0

mengqiy force-pushed the setElementOrder branch from 65b1632 to c3481b0 Compare May 31, 2017 22:08

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-github-robot merged commit 82245a1 into kubernetes:master Jun 1, 2017

mengqiy deleted the setElementOrder branch June 1, 2017 16:15

liggitt mentioned this pull request Jun 4, 2017

Millions of conflicts on Nodes in kubemark-scale tests #46851

Closed

mengqiy mentioned this pull request Jun 9, 2017

Kubectl apply should change the env variables order as per the new order #47248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support setElementOrder #45980

support setElementOrder #45980

mengqiy commented May 17, 2017 •

edited by pwittrock

mengqiy commented May 17, 2017 •

edited

mengqiy commented May 17, 2017

k8s-reviewable commented May 17, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 18, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 19, 2017

mengqiy commented May 22, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

mengqiy commented May 30, 2017

pwittrock commented May 31, 2017

pwittrock commented May 31, 2017

mengqiy commented May 31, 2017

pwittrock commented May 31, 2017

pwittrock commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented Jun 1, 2017

pwittrock commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

support setElementOrder #45980

support setElementOrder #45980

Conversation

mengqiy commented May 17, 2017 • edited by pwittrock

mengqiy commented May 17, 2017 • edited

mengqiy commented May 17, 2017

k8s-reviewable commented May 17, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 18, 2017

apelisse commented May 18, 2017

mengqiy commented May 18, 2017

apelisse commented May 19, 2017

mengqiy commented May 22, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

pwittrock commented May 30, 2017

mengqiy commented May 30, 2017

pwittrock commented May 31, 2017

pwittrock commented May 31, 2017

mengqiy commented May 31, 2017

pwittrock commented May 31, 2017

pwittrock commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented May 31, 2017

mengqiy commented Jun 1, 2017

pwittrock commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

mengqiy commented May 17, 2017 •

edited by pwittrock

mengqiy commented May 17, 2017 •

edited