-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
847d6c0
to
af46008
Compare
Co-authored-by: Faiq <faiq.raza@nutanix.com> Signed-off-by: Deepak Muley <deepak.muley@nutanix.com>
af46008
to
7e2b6e6
Compare
1b7ea37
to
c1695ce
Compare
@jimmidyson @faiq @dkoshkin @thunderboltsid any more comments? can we merge this? |
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 comments but this is very close to being merged! Great work @deepakm-ntnx!
charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/nutanix-cluster-class.yaml
Show resolved
Hide resolved
charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/nutanix-cluster-class.yaml
Show resolved
Hide resolved
charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/nutanix-cluster-class.yaml
Show resolved
Hide resolved
For now, I think it would be better to have non-working minimal examples here and later down the line adding the proper code in the form of GeneratePatch hooks and Lifecycle hooks to get started. |
yes thats the plan if i understand your comment correctly. is it not the case in this PR? |
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 for the tickets @deepakm-ntnx.
I'm good with merging this as is and working on those follow up tickets, but I will let @faiq and @jimmidyson do the actual approvals since they left the original comments.
we will remove ccm related things from the cluster-topology example yaml once we move ccm installation as addon thru post cluster creation step in ccm https://d2iq.atlassian.net/browse/D2IQ-99841 |
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.
I strongly prefer to remove the CCM cluster resource sets from the examples.
I can live with the rest of the stuff not being patches.
could you please add more details on why you feel so strongly? would it not be ok with you if we do it thru above mentioned tracker? i think this is really minor change and does not really add value. mainly https://d2iq.atlassian.net/browse/D2IQ-99841. given we are still in dev phase, we should run in iterations. cc @jimmidyson |
While I would prefer CCM to be added as an addon at this point, to keep this moving and to make it possible to create a Nutanix cluster using CAREN hooks, I agree we should merge this once it's working and do the refactoring as a follow up. This will unblock other work, such as testing Nutanix CSI, etc, and allow adding e2e tests, etc. |
charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/nutanix-cluster-class.yaml
Show resolved
Hide resolved
We don't need CCM to be enabled to be able to test CSI. We could simply test if the pods will come up on the first control plane node. |
OK but CCM is required to bring a cluster fully up and make all nodes ready so it is still a blocker for e2e tests for Nutanix provider for example. |
tests should fail until the code is complete. |
added nutanix examples for clusterclass and cluster