-
Notifications
You must be signed in to change notification settings - Fork 263
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
⚠️ Add v1alpha7 #1497
⚠️ Add v1alpha7 #1497
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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 |
/test pull-cluster-api-provider-openstack-e2e-full-test |
/test pull-cluster-api-provider-openstack-e2e-full-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
One nit below, and one question: Should we change the clusterctl upgrade test to initialize with v1alpha6 instead of v1alpha5 now?
Ref:
WorkloadFlavor: shared.FlavorV1alpha5, |
api/v1alpha6/conversion.go
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
Copyright 2022 The Kubernetes Authors. | |||
Copyright 2021 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where this came from. Maybe change it to 2023 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be after I deleted it and then copied it from elsewhere after realising that was a mistake 😂
I would defer this to you. We could:
|
Probably the nicest thing would be to add a second test and test both. But I would say it is more important to have v1alpha6 -> v1alpha7. |
/test pull-cluster-api-provider-openstack-e2e-full-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Ah missed that the e2e-full test was failing... |
/test pull-cluster-api-provider-openstack-e2e-full-test |
Thanks! I've added a second section. Is that going to work? |
Having both v0.6.4 and v0.7.1 is perfectly fine, but now I see two sections with v0.7.99. Not sure how that will go 🤔 |
🤦 |
/test pull-cluster-api-provider-openstack-e2e-full-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, only one nit I could notice is missing the API types update in the doc: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/docs/book/src/clusteropenstack/configuration.md
I'll let this -full test complete before pushing a docs update. Assuming everything is ok I won't bother running it again. |
@mdbooth makes sense 🙂 |
Only change is updating the api version refs in the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Not sure if the repo has requirements for the PR labels, but this is generally should be labelled as
/hold cancel |
Add a new v1alpha7 API version in anticipation of breaking API changes.
/hold