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

Update controller-runtime to 0.18.1 #13346

Merged
merged 13 commits into from May 22, 2024
Merged

Update controller-runtime to 0.18.1 #13346

merged 13 commits into from May 22, 2024

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented Apr 28, 2024 •
edited

What this PR does / why we need it:
This updates KKP to the new controller-runtime version. There is one major change that affected us: the generics support in the controller. Manually setting up Watch() calls means you have to use typed generic stuff, which is really verbose and a lot of boilerplate. I tried it with 3 controlllers and then quickly gave up. The code became unreadable.

It's much smarter to finally migrate everything to the controller builder, an abstraction we never used because back when KKP was written, the builder didn't support what we needed and then every time we created a new controller, we simply copy&pasted an existing one.

So this PR refactors every single controller in KKP to use the controller builder, which is a really nice interface and works perfectly for every controller in KKP.

What type of PR is this?
/kind chore

Does this PR introduce a user-facing change? Then add your Release Note here:

Update to controller-runtime 0.18.1

Documentation:

NONE

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 28, 2024
@kubermatic-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubermatic-bot kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/api Denotes a PR or issue as being assigned to SIG API. sig/app-management Denotes a PR or issue as being assigned to SIG App Management. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. sig/networking Denotes a PR or issue as being assigned to SIG Networking. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 28, 2024
@xrstf
Copy link
Contributor Author

xrstf commented Apr 28, 2024

/test pre-kubermatic-test

@xrstf xrstf self-assigned this Apr 28, 2024
@xrstf
Copy link
Contributor Author

xrstf commented Apr 28, 2024

/test all

@xrstf
Copy link
Contributor Author

xrstf commented Apr 28, 2024

/test all

@xrstf
Copy link
Contributor Author

xrstf commented Apr 28, 2024

/test pre-kubermatic-mla-e2e
/test pre-kubermatic-opa-e2e

@xrstf
Copy link
Contributor Author

xrstf commented Apr 29, 2024

/test pre-kubermatic-mla-e2e

@xrstf
Copy link
Contributor Author

xrstf commented Apr 29, 2024

/test all

@xrstf
Copy link
Contributor Author

xrstf commented Apr 29, 2024

/override pre-kubermatic-test-integration

@kubermatic-bot
Copy link
Contributor

@xrstf: Overrode contexts on behalf of xrstf: pre-kubermatic-test-integration

In response to this:

/override pre-kubermatic-test-integration

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.

@xrstf xrstf marked this pull request as ready for review April 29, 2024 08:15
@kubermatic-bot kubermatic-bot added docs/none Denotes a PR that doesn't need documentation (changes). release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 29, 2024
@xrstf
Copy link
Contributor Author

xrstf commented Apr 29, 2024

/override pre-kubermatic-test-integration

@kubermatic-bot
Copy link
Contributor

@xrstf: Overrode contexts on behalf of xrstf: pre-kubermatic-test-integration

In response to this:

/override pre-kubermatic-test-integration

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.

@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e2373d664acd2329b26232de2403812384a454da

@kubermatic-bot kubermatic-bot removed the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@xrstf
Copy link
Contributor Author

xrstf commented May 22, 2024

@kron4eg rebased 😁

@kron4eg
Copy link
Member

kron4eg commented May 22, 2024

/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: bc931a4606d195fd69097211a549dccc7ea947ef

@xrstf
Copy link
Contributor Author

xrstf commented May 22, 2024

/approve

1 similar comment
@Waseem826
Copy link
Contributor

/approve

cnvergence
cnvergence approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

/approve

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cnvergence, kron4eg, Waseem826, xrstf

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2024
@kubermatic-bot kubermatic-bot merged commit dd930a2 into kubermatic:main May 22, 2024
24 checks passed
@kubermatic-bot kubermatic-bot added this to the KKP 2.26 milestone May 22, 2024
@xrstf xrstf deleted the ctrl-runtime-0.18 branch May 22, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnvergence cnvergence cnvergence approved these changes

@kron4eg kron4eg Awaiting requested review from kron4eg

Assignees

@kron4eg kron4eg

@xrstf xrstf

@cnvergence cnvergence

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). lgtm 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. sig/api Denotes a PR or issue as being assigned to SIG API. sig/app-management Denotes a PR or issue as being assigned to SIG App Management. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. sig/networking Denotes a PR or issue as being assigned to SIG Networking. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
KKP 2.26
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@xrstf @kubermatic-bot @kron4eg @Waseem826 @cnvergence