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.0-beta.1 #2251

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

🌱 Bump CAPI to v1.9.0-beta.1 #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 fa6367c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/673e48130cd2a200089a27e2
😎 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 6 times, most recently from 640d8f3 to b84016a Compare November 20, 2024 19:24
@EmilienM
Bump to CAPI 1.9
e175496
@EmilienM
controllers: adapt for CAPI 1.9 breaking changes
47e380f 
Add schemes into predicates (see
kubernetes-sigs/cluster-api#11239)
@EmilienM
Import deprecated but needed errors constants from CAPI
fa6367c 
Context: kubernetes-sigs/cluster-api#10784
We'll maintain them here for the time being, until we have conditions
replacing these errors.
@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@mdbooth mdbooth Awaiting requested review from mdbooth

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. 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.
3 participants
@EmilienM @lentzi90 @k8s-ci-robot