-
Notifications
You must be signed in to change notification settings - Fork 16
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
✨ Support creating management v3 cluster #362
✨ Support creating management v3 cluster #362
Conversation
ef74d3e
to
fa01e18
Compare
9ab7beb
to
0de8141
Compare
f3988f0
to
dcacd4a
Compare
f74610b
to
463bf77
Compare
This is now updated and rebased with the recent changes to the test environment. |
Seems like this is now ready for a final review. Thanks everyone for taking the time to add your suggestions. |
Reminder that there's also this documentation PR rancher/turtles-docs#59 to document the new feature flag. |
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 nits, overall looks good
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 great, couple minor comments
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
9f4d45f
to
927cae7
Compare
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
927cae7
to
6c02634
Compare
What this PR does / why we need it:
This PR adds support for representing imported CAPI clusters as
management.cattle.io/v3
clusters instead of the currentprovisioning.cattle.io/v1
resource.ADR 8 contains the rationale behind choosing this implementation approach and the discussions on different alternatives before agreeing to a decision.
This includes a new import controller specific for the new type of resource, which is only enabled if using a feature flag, so the default behavior of Rancher Turtles is to continue with the current import strategy. Some of the functions that are used by both controllers have been centralized so that we have less code to maintain. Still, some of the controller code is very similar to one another but re-utilizing it would probably increase complexity and reduce readability.
An E2E specification equivalent to the existing
import gitops
has been created for when Rancher Turtles is executed with this feature flag enabled.There are definitely improvements to the way the code is structured. Especially in terms of increasing the re-usability, for both controller and E2E, but my opinion is that this PR is large enough to also add a larger refactor. Probably it would be better if we can have a couple of follow-ups to simplify the code where possible. But this is obviously a team decision. Additionally, there will be a discussion on whether we should move completely to this new controller and deprecate
provisioning.cattle.io
, so it may not make much sense to invest time in increasing re-usable components at this point.Which issue(s) this PR fixes:
Fixes #298
Special notes for your reviewer:
The DeletionTimestamp seems to not be set when the Rancher Cluster is deleted. The logic is the same that we use for other controller, though. I would appreciate any feedback on this. I'll leave a comment below.The new feature flag is added to the docs here.
Checklist: