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.

Sign up for GitHub

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

Jump to bottom

Add retainKeys to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy #50296

Merged
merged 3 commits into from
Aug 29, 2017
Merged

Add retainKeys to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy #50296

merged 3 commits into from
Aug 29, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Aug 8, 2017

Add retainKeys to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy.

With the new value in patchStrategy, the patch will include an optional directive that will tell the apiserver to clear defaulted fields and update. This will resolve issue like #34292 (comment) and similar issue caused by defaulting in volume.

The change is backward compatible.

The proposal for this new patch strategy is in https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md

The implementation to support the new patch strategy's logic is in #44597 and has been merged in 1.7.

Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy.

/assign @apelisse
/assign @janetkuo for deployment change
/assign @saad-ali for volume change

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 8, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Aug 8, 2017

/unassign @sttts @cblecker
/assign @apelisse @janetkuo @saad-ali

@k8s-ci-robot k8s-ci-robot assigned apelisse, janetkuo and saad-ali and unassigned sttts and cblecker Aug 8, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Aug 8, 2017

/retest

@mengqiy
Copy link
Member Author

mengqiy commented Aug 16, 2017

/assign @Kargakis
for deployment change

@mengqiy
Copy link
Member Author

mengqiy commented Aug 16, 2017

ping @saad-ali @apelisse @Kargakis for review.

@0xmichalis
Copy link
Contributor

I like this.

/cc @mfojtik @tnozicka since we have felt the pain before with validation when switching between strategies

@k8s-ci-robot k8s-ci-robot requested a review from tnozicka August 16, 2017 23:35
@k8s-ci-robot
Copy link
Contributor

@Kargakis: GitHub didn't allow me to request PR reviews from the following users: have, switching, pain, before, validation, between, the, with, when, strategies, we, felt, since.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

I like this.

/cc @mfojtik @tnozicka since we have felt the pain before with validation when switching between strategies

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested a review from mfojtik August 16, 2017 23:35
@0xmichalis
Copy link
Contributor

lgtm but I would like to read others thoughts

@mfojtik
Copy link
Contributor

mfojtik commented Aug 17, 2017

lgtm as well

@mengqiy
Copy link
Member Author

mengqiy commented Aug 22, 2017

ping @saad-ali @janetkuo

@mengqiy
Copy link
Member Author

mengqiy commented Aug 23, 2017

/assign @erictune @thockin
for api review.

@thockin
Copy link
Member

thockin commented Aug 24, 2017

I don't really understand the semantic, but I guess that is OK.

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2017
@thockin thockin removed their assignment Aug 24, 2017
@janetkuo janetkuo added this to the v1.8 milestone Aug 28, 2017
janetkuo
janetkuo reviewed Aug 28, 2017
View reviewed changes
hack/make-rules/test-cmd-util.sh Outdated
# defaulted fields and successfully update the deployment
kubectl apply -f hack/testdata/retainKeys/deployment/deployment-after.yaml "${kube_flags[@]}"
# Post-Condition: deployment "test-deployment-retainkeys" updated
kube::test::get_object_assert deployments "{{range.items}}{{$id_field}}{{end}}" 'test-deployment-retainkeys'
Copy link
Member

Choose a reason for hiding this comment

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

nit: this line does not tell you if deployment "test-deployment-retainkeys" is updated or not

@mengqiy mengqiy force-pushed the addApplyTestForReplacekeys branch from 19b79ff to 31c92d9 Compare August 28, 2017 22:44
@mengqiy
Copy link
Member Author

mengqiy commented Aug 28, 2017

@janetkuo Updated. PTAL. Thanks!

@janetkuo
Copy link
Member

This lgtm. You may squash commits.

@mengqiy mengqiy force-pushed the addApplyTestForReplacekeys branch from 31c92d9 to 93d2625 Compare August 28, 2017 23:22
@mengqiy mengqiy force-pushed the addApplyTestForReplacekeys branch from 93d2625 to 9b05e26 Compare August 28, 2017 23:38
@janetkuo
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, mengqiy, thockin

Associated issue: 34292

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@mengqiy
Copy link
Member Author

mengqiy commented Aug 29, 2017

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296)

@k8s-github-robot k8s-github-robot merged commit 80ea31f into kubernetes:master Aug 29, 2017
@mengqiy mengqiy deleted the addApplyTestForReplacekeys branch September 7, 2017 04:36
@liggitt liggitt mentioned this pull request Oct 30, 2018
Merged
@KnVerey KnVerey mentioned this pull request Oct 4, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

@tnozicka tnozicka Awaiting requested review from tnozicka

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@apelisse apelisse

@0xmichalis 0xmichalis

@janetkuo janetkuo

@erictune erictune

@saad-ali saad-ali

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.