-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Add info about patch strategy. #7286
Conversation
Deploy preview for kubernetes-io-master-staging ready! Built with commit 258db25 https://deploy-preview-7286--kubernetes-io-master-staging.netlify.com |
@@ -1,4 +1,4 @@ | |||
apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2 | |||
apiVersion: apps/v1beta2 # for versions before 1.9.0 use apps/v1beta2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change is needed? We'd better use (and encourage users to use) apps/v1
for 1.9+, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was experimenting and forgot to change it back to v1. It's back at v1 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some nits.
@@ -54,7 +54,7 @@ get terminated and replaced by new ones. | |||
At this point, each Pod has one Container that runs the nginx image. Now suppose | |||
you want each Pod to have two containers: one that runs nginx and one that runs redis. | |||
|
|||
Create a file named `patch-file.yaml` that has this content: | |||
Create a file named `patch-file-containers.yaml` that has this content: | |||
|
|||
```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are here, how about change this to "```yaml"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
existing list. This is not always what happens when you use a strategic merge patch on a list. | ||
In some cases, the list is replaced, not merged. | ||
|
||
With a strategic merge patch, a list is either replaced or merged depending it its |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it its
--> its
, an extra it
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
patch strategy. The patch strategy is specified by the value of the `patchStrategy` key | ||
in a field tag. For example, the `Containers` field of PodSpec has a `patchStrategy` of `merge`: | ||
|
||
```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/shell/go ?
You can also see the patch strategy in the | ||
[OpenApi spec](https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json): | ||
|
||
```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/shell/json ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
Create a file named `patch-file-tolerations.yaml` that has this content: | ||
|
||
```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/shell/yaml
the Tolerations field of PodSpec does not have a `patchStrategy` key in its filed tag. So the | ||
strategic merge patch uses the default patch strategy, which is `replace`. | ||
|
||
```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/shell/go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
52d4a53
to
d3485ab
Compare
d3485ab
to
258db25
Compare
@xiangpengzhao Thanks for the review. I think I've addressed all of your comments. |
/assign @chenopis |
Yup. That definitely resolves my complaint. |
/approve |
@xiangpengzhao Can you give this an LGTM? Thanks. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chenopis, steveperry-53, xiangpengzhao 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 OWNERS Files:
Approvers can indicate their approval by writing |
Addresses #6222
This change is