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

⚠️ Bump controller-runtime 0.13.1=>0.14.1 #7906

Conversation

aniruddha2000
Copy link
Contributor

Signed-off-by: Aniruddha Basak codewithaniruddha@gmail.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):
Part of #7671

cc @kubernetes-sigs/cluster-api-release-team

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 11, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 11, 2023
@sbueringer
Copy link
Member

/retitle ⚠️ Bump controller-runtime 0.13.1=>0.14.1

@k8s-ci-robot k8s-ci-robot changed the title Bump controller-runtime 0.13.1=>0.14.1 ⚠️ Bump controller-runtime 0.13.1=>0.14.1 Jan 11, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2023
@sbueringer sbueringer mentioned this pull request Jan 11, 2023
13 tasks
Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

Looks like this version of controller-runtime introduced some breaking changes.
There are a few lint issue that need to be addressed. Running the linter locally I see the following issues:

  • The ValidateLabelSelector function signature changed. Passing an empty options object (metav1validation.LabelSelectorValidationOptions{}) as the second argument, at every use, should be enough to fix that.
  • sets.String seems to be deprecated and now the recommendation seems to be to use generics to create a set. Doing that switch is out of scope for this PR. Let's do it in a separate PR. So for now, I would suggest adding an exception in the linter for the scope of this PR and address it in a follow up PR.
  • controller-runtime's client.Client interface changes and the dryrun.Client struct no longer implements the interface correctly. I would suggest adding the required functions to the dryrun.Client struct should be enough for this PR. I will need more time to see if we can get away with an "empty" implmentation as the dryrun client does not operate on subresources.

@aniruddha2000 aniruddha2000 force-pushed the ani/bump-controller-runtime-0.14.1 branch from 28fc3fb to fb937ea Compare January 13, 2023 07:04
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2023
.golangci.yml Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

Changes lgtm pending what is mentioned in this thread: #7906 (comment)

@aniruddha2000
Copy link
Contributor Author

Lint passed but seems there are a lot of complicated breaking changes now 😵‍💫

@sbueringer
Copy link
Member

sbueringer commented Jan 17, 2023

@aniruddha2000 I can take a look at it and then open a PR against your branch if you want

@aniruddha2000
Copy link
Contributor Author

@sbueringer sure!

@sbueringer
Copy link
Member

sbueringer commented Jan 17, 2023

@sbueringer sure!

Will do. Looks like some of the failures are related to some things I identified once I got a chance to catch up with controller-runtime yesterday (#7925)

@sbueringer
Copy link
Member

sbueringer commented Jan 17, 2023

@aniruddha2000 PR is open here: aniruddha2000#10
Maybe just merge it so my change will end up on the current PR.

Then we can take it from there.

The problem was essentially that fake client now supports indexes and we have to add them to the fake client explicitly

@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

Thx!
Can you please squash the commits?

/lgtm

/assign @fabriziopandini @ykakarap

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

LGTM label has been added.

Git tree hash: e69272945ee3f1e81fd68c7d4c2d0bb1244c9df3

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

LGTM pending squash.

Just minor nits.

Thank you @aniruddha2000 for all the work! 🚀

internal/controllers/machine/machine_controller_test.go Outdated Show resolved Hide resolved
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

.golangci.yml Show resolved Hide resolved
Signed-off-by: Aniruddha Basak <codewithaniruddha@gmail.com>
@aniruddha2000 aniruddha2000 force-pushed the ani/bump-controller-runtime-0.14.1 branch from 71e207e to 0a62991 Compare January 19, 2023 06:03
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2023
@aniruddha2000
Copy link
Contributor Author

Squash done

@sbueringer
Copy link
Member

/lgtm
/approve

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

LGTM label has been added.

Git tree hash: 8ee3d6f0046b86c2177f0dc6db4aa026cf102135

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 Jan 19, 2023
@k8s-ci-robot k8s-ci-robot merged commit 725addb into kubernetes-sigs:main Jan 19, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Jan 19, 2023
@RainbowMango
Copy link
Member

Hi @kubernetes-sigs/cluster-api-release-team,

Could you please tag a new patch release to include this?

We(Karmada project) are going to bump Kubernetes(v1.26) and controller-runtime(v0.14.1), but we are blocked by cluster-api because we are using cluster-api@v1.0.1.

I checked the latest release v1.3.2 which does not include this patch.
I'd like a new tagged release from cluster-api, like v1.3.3?

# sigs.k8s.io/cluster-api/api/v1beta1
vendor/sigs.k8s.io/cluster-api/api/v1beta1/machineset_types.go:180:83: not enough arguments in call to metav1validation.ValidateLabelSelector
	have (*"k8s.io/apimachinery/pkg/apis/meta/v1".LabelSelector, *field.Path)
	want (*"k8s.io/apimachinery/pkg/apis/meta/v1".LabelSelector, validation.LabelSelectorValidationOptions, *field.Path)

@killianmuldoon
Copy link
Contributor

@RainbowMango according to the release timeline - https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/release/release-1.4.md#timeline - there should be release of v1.3.3 this week.

@furkatgofurov7
Copy link
Member

@RainbowMango according to the release timeline - https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/release/release-1.4.md#timeline - there should be release of v1.3.3 this week.

@killianmuldoon this was merged to main and not back-ported to release-1.3 branch AFAIU, so I would not expect to have these changes in the new patch release (v1.3.3).

@RainbowMango
Copy link
Member

OK, that makes sense. For now, I can use a pseudo version from the main branch(like v1.3.0-rc.0.0.20230127161026-14ffcb25bbf2), and change it to v1.4.0 after v1.4.0 is released.

Thanks @killianmuldoon @furkatgofurov7. That really helps.

@sbueringer
Copy link
Member

For clarification. We consider bumping controller runtime and k8s.io/* dependencies breaking changes. Thus this won't be backported.

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

Successfully merging this pull request may close these issues.

None yet

8 participants