-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 support for add-on providers in clusterctl #8472
✨ Add support for add-on providers in clusterctl #8472
Conversation
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 at first glance - will give it a proper review and test later in the week.
@fabriziopandini I based this off of #7288, what are your thoughts on adding an add-on provider type so we can set up CAAPH with clusterctl init? |
0e4d9e2
to
30b4e45
Compare
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.
great to see this moving!
I will start by harmonizing usage of addon
and add-on
across the board, then we can nail down other nits progressively
726a812
to
a502e50
Compare
a502e50
to
6a95c43
Compare
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.
Did an initial pass and overall it seems okay. I will do a full pass and test this locally.
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.
Some notes:
- No action needed but I wanted highlight it just in case:
clusterctl upgrade plan/apply
will not suggest upgrade for CAAPH right because all the versions of CAAPH are pre-release versions. If a user is onv0.1.0-alpha.1
clusterctl will report it as on the "latest version" and users will have to manually upgrade. This is working as expected and we do not want to change the behavior but just wanted to highlight it here.clusterctl init
will pick up the latest alpha so clusterctl init --addon helm should work. clusterctl_upgrade.go
e2e should be modified to support addons.
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.
Some notes:
- No action needed but I wanted highlight it just in case:
clusterctl upgrade plan/apply
will not suggest upgrade for CAAPH right because all the versions of CAAPH are pre-release versions. If a user is onv0.1.0-alpha.1
clusterctl will report it as on the "latest version" and users will have to manually upgrade. This is working as expected and we do not want to change the behavior but just wanted to highlight it here.clusterctl init
will pick up the latest alpha so clusterctl init --addon helm should work. clusterctl_upgrade.go
e2e should be modified to support addons.
@ykakarap Went over some of these changes. WDYT? |
/test ? |
@ykakarap: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
/test pull-cluster-api-e2e-full-main |
Overall looks good. I think this is getting close to ready. |
980da14
to
b743994
Compare
/test pull-cluster-api-e2e-full-main |
b743994
to
3486493
Compare
Overall looks good. LGTM pending tests fix. |
lgtm pending test fix + confirmation on #8472 (comment) |
3486493
to
fc125b0
Compare
Except for a few comments looks like we are using @Jont828 Thank you adding addon support to clusterctl! Great work 🚀 /lgtm |
LGTM label has been added. Git tree hash: cb998de5c72966f0851ff2565ce9fb1901c88a9e
|
/hold until the release file in CAAPH is renamed to |
/approve |
[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 |
File is correct now. @Jont828 Thank you very much for working on this!! 🎉 /hold cancel |
What this PR does / why we need it: Add support for add-on providers in clusterctl so we can use CAAPH with clusterctl init
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #