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 CAPI to v1.9.3 #2251

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Bump CAPI to v1.9.3 #2251

wants to merge 3 commits into from

Conversation

EmilienM
Copy link
Contributor

@EmilienM EmilienM commented Nov 12, 2024 •
edited
Loading

What this PR does / why we need it:

Bump to the latest CAPI:

  • Updates go.mod with the new tag.
  • Run make modules in root and orc/.
  • Run make generate.
  • Add new API violations from what has been generated.
  • Add schemes into predicates (see ⚠️ Fix object logging in predicates cluster-api#11239)
  • Import deprecated errors that are used in CAPO and make it explicit that they are deprecated and will be replaced by Conditions when time permits.

Issue #2259

@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 Nov 12, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 12, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 25f4b24
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6772b06e0d5b8a00087d7a83
😎 Deploy Preview https://deploy-preview-2251--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@EmilienM
Copy link
Contributor Author

/hold
it's still in beta, but at least we have it ready and we can see how it works in our CI.

@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 Nov 12, 2024
@EmilienM
Copy link
Contributor Author

EmilienM commented Nov 12, 2024 •
edited
Loading

The tests fail because kubernetes-sigs/cluster-api#11317 which deprecates Failure* fields:
https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-openstack/2251/pull-cluster-api-provider-openstack-test/1856337619219124224/build-log.txt

@lentzi90 @mdbooth we should get rid of the FailureReason and FailureMessage. Do we want a transition step where we carry this interface in CAPO or switch to conditions to surface errors, outside of this PR in preparation of a bump to this version of CAPI?

@lentzi90
Copy link
Contributor

I think it depends on how much work it would be to switch. If we can do that pretty easily, let's just get it done. Otherwise, let's ignore the deprecation warning and give ourselves more time to work on it in the mean time.

@EmilienM EmilienM changed the title 🌱 Bump CAPI to v1.9.0-beta.0 🌱 Bump CAPI to v1.9.0-beta.1 Nov 20, 2024
@EmilienM EmilienM force-pushed the capi-1.9-beta branch 7 times, most recently from b84016a to fa6367c Compare November 20, 2024 20:35
@EmilienM
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024
@EmilienM
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM

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 Nov 20, 2024
lentzi90
lentzi90 reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Great job!

Do we want to take this in already or wait for the final release?
One thing I am wondering about is also how the FailureMessage will now work. Previously, CAPI would propagate them up to the CAPI objects, but I'm guessing that would no longer happen in v1.9. That would mean that it is harder for users to detect when something is wrong.
If that it the case, should we rather hold off on this until we can transition to conditions instead?

@EmilienM
Copy link
Contributor Author

Great job!

Do we want to take this in already or wait for the final release? One thing I am wondering about is also how the FailureMessage will now work. Previously, CAPI would propagate them up to the CAPI objects, but I'm guessing that would no longer happen in v1.9. That would mean that it is harder for users to detect when something is wrong. If that it the case, should we rather hold off on this until we can transition to conditions instead?

Yeah, that's a good point. I'm personally convinced that we will not release 0.12 until we get these conditions addressed, so we can probably live without the CAPO failures propagated to the CAPI objects.
Some platforms have bumped already (VSphere) but I suspect they already got rid of the conditions.
I'll let you and @mdbooth decide, but I'm in favour of bumping sooner rather than later.

@lentzi90
Copy link
Contributor

Yeah I agree it is good to bump early and test it out properly. Shouldn't be a big deal to switch to conditions either.

@EmilienM
Copy link
Contributor Author

@mdbooth Matt, what do you think?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2024
@EmilienM
Copy link
Contributor Author

TODO: make changes to release notes generator tool in Makefile, the new version in CAPI 1.9 is a bit different. See how VSphere is doing it.
Reference: #2289

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2024
@EmilienM
Copy link
Contributor Author

During the CAPO meeting this week, we agreed we could merge this one and iterate over #2290 as soon as possible.

@EmilienM EmilienM changed the title 🌱 Bump CAPI to v1.9.0-beta.1 🌱 Bump CAPI to v1.9.0 Dec 11, 2024
@EmilienM EmilienM force-pushed the capi-1.9-beta branch 3 times, most recently from 9110f83 to 1565041 Compare December 12, 2024 16:03
@EmilienM EmilienM requested a review from lentzi90 December 12, 2024 18:31
@EmilienM
Copy link
Contributor Author

@mdbooth @lentzi90 at your discretion to merge it.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2024
@EmilienM @MaysaMacedo
Bump to CAPI 1.9.3
ccaefa3
@EmilienM @MaysaMacedo
controllers: adapt for CAPI 1.9 breaking changes
f40d9bf 
Add schemes into predicates (see
kubernetes-sigs/cluster-api#11239)
@EmilienM @MaysaMacedo
Import deprecated but needed errors constants from CAPI
25f4b24 
Context: kubernetes-sigs/cluster-api#10784
We'll maintain them here for the time being, until we have conditions
replacing these errors.
@MaysaMacedo
Copy link
Contributor

/retitle Bump CAPI to v1.9.3

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Bump CAPI to v1.9.0 Bump CAPI to v1.9.3 Dec 30, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdbooth mdbooth Awaiting requested review from mdbooth

@lentzi90 lentzi90 Awaiting requested review from lentzi90

Assignees
No one assigned
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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
CAPO Roadmap
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EmilienM @lentzi90 @k8s-ci-robot @MaysaMacedo