Skip to content
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 searching for CAPI clusters in the current context #154

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

eljohnson92
Copy link
Contributor

This PR allows you to passively search any cluster in your current context for CAPI clusters if a kubeconfigPath is not defined. This makes this store more flexible as mgmt clusters are deleted, created, and switched between. Open to any feedback on any explanations or implementations here. an alternative to this is we could add an explicit useCurrentContext: true bool or something like that

Copy link
Owner

@danielfoehrKn danielfoehrKn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of optionally searching for CAPI clusters in the current context. So you could just switch to the management cluster using kubeswitch to then discover the CAPI clusters there (instead of defining a store per management cluster if that does not fit your workflow)

}
return
}
s.Logger.Debug("CAPI: cannot listing v1beta1.Cluster resources, not currently connected to a cluster with CAPI installed")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably would be good to check the error string to if it indicates missing Cluster CRD. Or directly probe the K8s API for the Cluster CRD.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea, playing around with some ways to do that now, I'll have an update shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be fixed to return an error if there is an error other than the CRD not being found

Copy link
Owner

@danielfoehrKn danielfoehrKn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@danielfoehrKn danielfoehrKn merged commit fcda663 into danielfoehrKn:master Dec 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants