Skip to content
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

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

steveperry-53
Copy link
Contributor

@steveperry-53 steveperry-53 commented Feb 8, 2018

Addresses #6222

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2018
@steveperry-53 steveperry-53 added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2018
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Feb 8, 2018

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@xiangpengzhao xiangpengzhao left a 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
Copy link
Contributor

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"?

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/shell/json ?

Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/shell/go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018
@steveperry-53 steveperry-53 force-pushed the strat-merge branch 2 times, most recently from 52d4a53 to d3485ab Compare February 8, 2018 21:32
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 8, 2018
@steveperry-53
Copy link
Contributor Author

@danwinship

@steveperry-53
Copy link
Contributor Author

@xiangpengzhao Thanks for the review. I think I've addressed all of your comments.

@steveperry-53 steveperry-53 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2018
@steveperry-53
Copy link
Contributor Author

/assign @chenopis

@danwinship
Copy link
Contributor

Yup. That definitely resolves my complaint.

@chenopis
Copy link
Contributor

/approve

@steveperry-53
Copy link
Contributor Author

@xiangpengzhao Can you give this an LGTM? Thanks.

@xiangpengzhao
Copy link
Contributor

/lgtm
💯

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2018
@k8s-ci-robot
Copy link
Contributor

[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:
  • OWNERS [chenopis,steveperry-53]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c46425d into kubernetes:master Feb 13, 2018
@steveperry-53 steveperry-53 deleted the strat-merge branch February 15, 2018 20:15
tehut pushed a commit to tehut/website that referenced this pull request Feb 20, 2018
bsalamat pushed a commit to bsalamat/kubernetes.github.io that referenced this pull request Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants