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

Bump to SMD PR#166 to pick up unsetting field changes and add integration tests #92661

Merged
merged 3 commits into from
Jul 10, 2020
Merged

Bump to SMD PR#166 to pick up unsetting field changes and add integration tests #92661

merged 3 commits into from
Jul 10, 2020

Conversation

jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Jun 30, 2020 •
edited by apelisse
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Picks up kubernetes-sigs/structured-merge-diff#166. See also the Design Proposal.

Fixes #92661

Special notes for your reviewer:

Vendor change:

hack/pin-dependency.sh sigs.k8s.io/structured-merge-diff/v3 43c19bbb7fba66396555aeebbec8227d024220ab
hack/update-vendor.sh

Note that SMD has comprehensive testing (kubernetes-sigs/structured-merge-diff#166). The additional integration tests here are to ensure that specific end-to-end cases we are about are working as expected.

Does this PR introduce a user-facing change?:

Server-side apply behavior has been regularized in the case where a field is removed from the applied configuration. Removed fields which have no other owners are deleted from the live object, or reset to their default value if they have one. Safe ownership transfers, such as the transfer of a `replicas` field from a user to an HPA without resetting to the default value are documented in [Transferring Ownership](https://kubernetes.io/docs/reference/using-api/api-concepts/#transferring-ownership)

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @apelisse
/priority important-soon

@k8s-ci-robot k8s-ci-robot requested a review from apelisse June 30, 2020 19:15
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 30, 2020
@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 30, 2020

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2020
apelisse
apelisse reviewed Jun 30, 2020
View reviewed changes
Copy link
Member

@apelisse apelisse left a comment

Choose a reason for hiding this comment

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

Awesome, I understand that you want to add specific tests here, right?

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go Outdated Show resolved Hide resolved
@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 30, 2020

Awesome, I understand that you want to add specific tests here, right?

Yes. I opened the PR before adding new tests to see how much impact the chang has one existing tests. I'll be adding some shortly.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 1, 2020

/retest

TestMaxResourceSize failed in pull-kubernetes-integration but passes locally.

@jpbetz jpbetz force-pushed the smd-bump-field-unsetting branch from b6b165f to 60b324a Compare July 2, 2020 15:28
@jpbetz jpbetz mentioned this pull request Jul 2, 2020
Merged
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2020
@jpbetz jpbetz force-pushed the smd-bump-field-unsetting branch from 57c21cf to 0e36907 Compare July 6, 2020 15:39
@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

2 similar comments
@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

@apelisse apelisse mentioned this pull request Jul 7, 2020
Closed
@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

2 similar comments
@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 7, 2020

/retest

I don't know what's going on with pull-kubernetes-e2e-kind...

@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.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 8, 2020

/test pull-kubernetes-kubemark-e2e-gce-big

@lavalamp lavalamp added this to the v1.19 milestone Jul 8, 2020
@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 9, 2020

rebased

@jpbetz jpbetz force-pushed the smd-bump-field-unsetting branch from 3fada77 to 3c2842f Compare July 9, 2020 13:19
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 9, 2020 •
edited
Loading

@jpbetz: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kind-ipv6 b6b165f37a54cc8c17ad90701db2b12323aaee39 link /test pull-kubernetes-e2e-kind-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jul 9, 2020

/retest

@lavalamp
Copy link
Member

lavalamp commented Jul 9, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2020
@apelisse apelisse mentioned this pull request Jul 9, 2020
Closed
@k8s-ci-robot k8s-ci-robot merged commit 49dced7 into kubernetes:master Jul 10, 2020
@thaJeztah
Copy link
Contributor

@lavalamp @apelisse are there plans to tag a new release of https://github.com/kubernetes-sigs/structured-merge-diff ? I noticed go.mod is now using a non-released version.

@apelisse
Copy link
Member

apelisse commented Aug 5, 2020

Yes, thanks, I can do it now.

@apelisse
Copy link
Member

apelisse commented Aug 5, 2020

Joe, since you've made that change, would you know if we need to bump the major or minor number?

@apelisse
Copy link
Member

apelisse commented Aug 5, 2020

OK, Talked to Joe, and there are some changes that are important enough that I'll bump the major.

@apelisse
Copy link
Member

apelisse commented Aug 5, 2020

We need to make a new PR against K8s to use that new tag.

@apelisse
Copy link
Member

apelisse commented Aug 5, 2020

Probably against the 1.19 release.

@captainroy-hy captainroy-hy mentioned this pull request Jan 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apelisse apelisse apelisse left review comments

@lavalamp lavalamp lavalamp left review comments

Assignees

@lavalamp lavalamp

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
6 participants
@jpbetz @lavalamp @apelisse @k8s-ci-robot @fejta-bot @thaJeztah