improve replaceKeys proposal #620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @pwittrock |
| - All of the missing fields will be cleared when patching. | ||
| - All fields in the `$replaceKeys` list must be a superset or the same as the fields present in the patch. | ||
|
|
||
| An new patch will have same content as old patch and an additional new directive. |
There was a problem hiding this comment.
-An new patch will have same content as old patch and an additional new directive.
+A new patch will have the same content as the old patch and an additional new directive.I'm not sure how carefully you want this to be proofread. There are some minor syntax issues I'm happy to fix if you think that would be useful.
There was a problem hiding this comment.
That will be great. Thanks!
mengqiy
force-pushed
the
update_replaceKeys_proposal
branch
from
1bfef0d
to
5339586
Compare
| Add tags `patchStrategy:"replaceKeys"`. | ||
| For a given type that has the tag, all keys/fields missing | ||
| from the request will be cleared when patching the object. | ||
| For a field presents in the request, it will be merged with the live config. |
There was a problem hiding this comment.
I'd replace this line with:
Each field present in the request will be merged with the live config.
|
|
||
| ## Analysis | ||
|
|
||
| There are 2 reasons of not choosing this logic: |
There was a problem hiding this comment.
Replace of not choosing with to avoid
| Reason is that `$patch` has been used in early releases. | ||
| If we add new value to this directive, | ||
| the old server will reject the new patch due to not knowing the new value. | ||
| - The patch have to include the entire the struct to hold the place in a list with `replace` patch strategy, |
There was a problem hiding this comment.
s/the entire the struct/the entire struct
| If we add new value to this directive, | ||
| the old server will reject the new patch due to not knowing the new value. | ||
| - The patch have to include the entire the struct to hold the place in a list with `replace` patch strategy, | ||
| even though there maybe no changes at all. |
| the old server will reject the new patch due to not knowing the new value. | ||
| - The patch have to include the entire the struct to hold the place in a list with `replace` patch strategy, | ||
| even though there maybe no changes at all. | ||
| This is less efficient comparing with the approach above. |
There was a problem hiding this comment.
s/comparing with/compared to
| @@ -224,8 +270,8 @@ Changes about how to generate the patch rely on package Strategic Merge Patch. | |||
| Strategic Merge Patch is a package used by both client and server. A typical usage is that a client | |||
| calls the function to calculate the patch and the API server calls another function to merge the patch. | |||
|
|
|||
| We need to make sure the client always sends a patch that includes all of the fields that it wants to keep. | |||
| When merging, auto clear missing fields of a patch if the patch has a directive `$patch: replaceKeys` | |||
| We need to make sure the new client always sends a patch with `$replaceKeys` directive. | |||
There was a problem hiding this comment.
s/sends a patch with/sends its patches with the
mengqiy
force-pushed
the
update_replaceKeys_proposal
branch
from
5339586
to
c914c7b
Compare
|
Updated. |
|
no problem! |
k8s-github-robot
pushed a commit
to kubernetes/kubernetes
that referenced
this pull request
May 27, 2017
Automatic merge from submit-queue (batch tested with PRs 46302, 44597, 44742, 46554) support replaceKeys patch strategy Implementing according to [this proposal](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md). The revision is in kubernetes/community#620. ```release-note support replaceKeys patch strategy and directive for strategic merge patch ```
mengqiy
force-pushed
the
update_replaceKeys_proposal
branch
from
c914c7b
to
588ce36
Compare
|
@pwittrock Can we merge this? |
pwittrock
reviewed
Jun 15, 2017
| bar: x | ||
| ``` | ||
|
|
||
| #### When the `$retainKeys` list has fields that not present in the patch |
There was a problem hiding this comment.
that not present in the patch
that are not present in the patch
pwittrock
reviewed
Jun 15, 2017
| There are 2 reasons of avoiding this logic: | ||
| - Using `$patch` as directive key will break backward compatibility. | ||
| But can easily fixed by using a different key, e.g. `retainKeys: true`. | ||
| Reason is that `$patch` has been used in early releases. |
There was a problem hiding this comment.
early releases
earlier releases
pwittrock
reviewed
Jun 15, 2017
| Reason is that `$patch` has been used in early releases. | ||
| If we add new value to this directive, | ||
| the old server will reject the new patch due to not knowing the new value. | ||
| - The patch have to include the entire struct to hold the place in a list with `replace` patch strategy, |
pwittrock
reviewed
Jun 15, 2017
| the old server will reject the new patch due to not knowing the new value. | ||
| - The patch have to include the entire struct to hold the place in a list with `replace` patch strategy, | ||
| even though there may be no changes at all. | ||
| This is less efficient comparing to the approach above. |
There was a problem hiding this comment.
efficient comparing
efficient compared
|
Minor updates. Otherwise LGTM |
mengqiy
force-pushed
the
update_replaceKeys_proposal
branch
from
588ce36
to
edcb7c5
Compare
|
Updated. PTAL |
MadhavJivrajani
pushed a commit
to MadhavJivrajani/community
that referenced
this pull request
Nov 30, 2021
…posal improve replaceKeys proposal
akhilerm
pushed a commit
to akhilerm/apimachinery
that referenced
this pull request
Sep 20, 2022
Automatic merge from submit-queue (batch tested with PRs 46302, 44597, 44742, 46554) support replaceKeys patch strategy Implementing according to [this proposal](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md). The revision is in kubernetes/community#620. ```release-note support replaceKeys patch strategy and directive for strategic merge patch ``` Kubernetes-commit: 6927e7061b129f88b718a5d78dfa8ce7233f384c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When implementing #278 (PR is kubernetes/kubernetes#44597),
@pwittrock and I found we can do it more efficiently when patching a list with
replacepatch strategy and still keep backward compatibility.This is very similar to what we have in #278.
cc: @liggitt