-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement adoption of k8s clusters #725
base: main
Are you sure you want to change the base?
Conversation
bc012d7
to
fab454f
Compare
6193f19
to
cf72c8f
Compare
@kylewuolle can the PR be split into two different PRs (or at least 2 different commits within the PR): the one with the renaming and the one with the adopted templates feature? Otherwise, there are too many changes to review and each requires attention because one might be either a simple renaming (which is pretty simple to review) or a feature (which requires extra attention) |
cf72c8f
to
3c982aa
Compare
Done! |
3c982aa
to
047904b
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.
Mostly, minor comments. I have a generic question: so the whole approach is to provide a template with an external (third-party, BYO) cluster, so it could be attached to the HMC but without handling any actions related to the provisioning and its managing, right?
I also have a concern regarding the provider name infrastracture-internal
, I am not aware how it should be named in terms of CAPI, I'd consult with other team members regarding it
templates/provider/hmc/templates/crds/hmc.mirantis.com_managedclusters.yaml
Outdated
Show resolved
Hide resolved
d1db299
to
9a565e5
Compare
Open to ideas for sure. I wanted to stay away from infrastructure-hmc since we'll be renaming that so not sure what else would be appropriate. |
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, I'd wait for the ideas regarding the infrastructure-internal
provider name and maybe update the ClusterDeployment
aliases to something like cldeploy
(the only alias, cld
might potentially interfere with other resources one day) but it is a personal preference (clusterd
sounds similar to a cluster daemon)
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.
Mostly minor comments.
Ideally, before creating the ClusterDeployment
with the adopted template, we should ensure that Sveltos is ready (because the template includes objects from the Sveltos API). For example, consider the template as invalid if the Sveltos is not ready yet (by the template controller) and then validate it in the webhook on a clusterdeployment
creation, similarly to the current approach with the providers' readiness. But sveltos is not a provider and we probably should have a separate annotation for that and implement a separate logic in the controller. Anyway, It’s worth gathering more opinions on this approach.
templates/cluster/adopted-cluster/templates/sveltoscluster.yaml
Outdated
Show resolved
Hide resolved
templates/cluster/adopted-cluster/templates/sveltoscluster.yaml
Outdated
Show resolved
Hide resolved
ef5ac7f
to
5b42254
Compare
@Kshatrix Agreed, this looks good from my perspective, please go ahead. |
4be0581
to
b216aa6
Compare
295c879
to
f96ce95
Compare
This PR resolves issue #542 by:
I've added a PR to update the documentation : Mirantis/project-2a-docs#68