-
Notifications
You must be signed in to change notification settings - Fork 2
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
Capi self filtering #28
Conversation
Add currentClusterName to acd spec
…r name of cluster to be skipped Update capi sample acd to include currentClusterRef
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 think the cluster ref should be within CAPI configuration.
|
||
// Current Cluster name indicating the management cluster | ||
// used to avoid choosing the cluster the controller is running in | ||
CurrentClusterRef Cluster `json:"currentClusterRef,omitempty"` |
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.
Should we add a CAPI field and configure this within that?
Given that we only use this for the CAPI Provider?
// +required | ||
Name string `json:"name"` | ||
} | ||
|
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.
One idea here is to implement Stringer this way in CAPIProvider.ClusterID
you could just call p.ManagementClusterRef.String()
if/when Cluster grows a Namespace, that code remains the same?
… management cluster name Add String funct to Cluster to convert the CurrentClusterRef to string whenever needed
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.
Couple of tidyups, and let's get your last branch of the year merged :-D
|
||
// String returns the string representation of the Cluster | ||
func (c Cluster) String() string { | ||
return fmt.Sprintf("%v", c.Name) |
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.
Could just return c.Name
?
@@ -45,6 +65,9 @@ type AutomatedClusterDiscoverySpec struct { | |||
// AKS configures discovery of AKS clusters from Azure. | |||
AKS *AKS `json:"aks,omitempty"` | |||
|
|||
// CAPI configures discovery of CAPI clusters | |||
CAPI *CAPI `json:"capi,omitempty"` |
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.
👍🏻
Issue weaveworks/weave-gitops-enterprise#3739
CurrentClusterRef
to acd spec to include the current management clustermanagementClusterRef
to capi