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

⚠️ Upgrade to controller-runtime v0.10, controller-tools v0.7 and Kubernetes v1.22 #5249

Merged
merged 1 commit into from Sep 21, 2021
Merged

⚠️ Upgrade to controller-runtime v0.10, controller-tools v0.7 and Kubernetes v1.22 #5249

merged 1 commit into from Sep 21, 2021

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Sep 16, 2021 •
edited

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5245
Now also fixes #5277

WIP => debugging tests

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2021
@sbueringer sbueringer changed the title ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes 1.22 ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 Sep 16, 2021
@sbueringer sbueringer changed the title ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 WIP: ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 Sep 16, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021
sbueringer
sbueringer commented Sep 16, 2021
View reviewed changes
controllers/machine_controller_node_labels_test.go
g.Expect(env.Create(ctx, machine)).To(Succeed())
// Note: We have to DeepCopy the machine, because the Create call clears the status and
// reconcileInterruptibleNodeLabel requires .status.nodeRef to be set.
g.Expect(env.Create(ctx, machine.DeepCopy())).To(Succeed())
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this was an intended change in controller-runtime: kubernetes-sigs/controller-runtime#1640

sbueringer
sbueringer commented Sep 16, 2021
View reviewed changes
controllers/remote/cluster_cache_tracker_test.go
Name: "test-cluster",
Annotations: make(map[string]string),
Namespace: ns.GetName(),
Name: "test-cluster",
},
}
g.Expect(k8sClient.Create(ctx, clusterA)).To(Succeed())
Copy link
Member Author

Choose a reason for hiding this comment

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

The Create call updates the empty map to nil.

I assume also a consequence of this change kubernetes-sigs/controller-runtime#1640

sbueringer
sbueringer commented Sep 16, 2021
View reviewed changes
internal/envtest/webhooks.go
validatingWebhooks := []client.Object{}
mutatingWebhooks := []client.Object{}
validatingWebhooks := []admissionv1.ValidatingWebhookConfiguration{}
mutatingWebhooks := []admissionv1.MutatingWebhookConfiguration{}

Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary because WebhookInstallOptions now expects the correct types and not just client.Object

@sbueringer
Copy link
Member Author

sbueringer commented Sep 16, 2021 •
edited

To fix the remaining test failures we need: kubernetes-sigs/controller-runtime#1666

With that fix, I had successful tests locally (via replace sigs.k8s.io/controller-runtime v0.10.0 => /Users/buringerst/code/src/sigs.k8s.io/controller-runtime)

I don't see a good workaround/alternative.

@fabriziopandini
Copy link
Member

lgtm pending 1666

@sbueringer sbueringer changed the title WIP: ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 Sep 16, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021
@sbueringer
Copy link
Member Author

I'm now using the latest commit of the CR main branch with my merged PRs, should all work now.

@CecileRobertMichon
Copy link
Contributor

is this on hold until the code freeze starts? given that it's breaking

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2021
@sbueringer sbueringer changed the title ⚠️ Upgrade to controller-runtime v0.10 and Kubernetes v1.22 ⚠️ Upgrade to controller-runtime v0.10, controller-tools v0.7 and Kubernetes v1.22 Sep 20, 2021
fabriziopandini
fabriziopandini reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

/lgtm

go.mod Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 20, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 21, 2021
@sbueringer sbueringer mentioned this pull request Sep 21, 2021
Closed
@sbueringer
Copy link
Member Author

flake + GitHub download error
/retest

@fabriziopandini
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 Sep 21, 2021
@killianmuldoon killianmuldoon mentioned this pull request Sep 21, 2021
Closed
@k8s-ci-robot @sbueringer
Merge pull request kubernetes-sigs#5270 from ykakarap/git-version-loo…
81c140d 
…kup-fix

🐛 Release Tooling: Version calculation script should only use annotated tags
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021
@sbueringer
Copy link
Member Author

This PR now uses the latest CR v0.10.1 release. I updated the PR description to also close #5277

fabriziopandini
fabriziopandini reviewed Sep 21, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021
vincepri
vincepri approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 files:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit c6ea715 into kubernetes-sigs:master Sep 21, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Sep 21, 2021
@sbueringer sbueringer deleted the pr-upgrade-controller-runtime branch September 21, 2021 18:47
@Ankitasw Ankitasw mentioned this pull request Sep 23, 2021
Merged
4 tasks
This was referenced Sep 27, 2021
Merged
Merged
@wallrj wallrj mentioned this pull request Feb 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@vincepri vincepri vincepri approved these changes

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

Assignees

@fabriziopandini fabriziopandini

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Use Controller-Runtime release for CAPI v1beta1/v1.0.0 Upgrade to Controller Runtime v0.10
5 participants
@sbueringer @fabriziopandini @CecileRobertMichon @k8s-ci-robot @vincepri