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

Capi self filtering #28

Merged
merged 4 commits into from
Dec 21, 2023
Merged

Capi self filtering #28

merged 4 commits into from
Dec 21, 2023

Conversation

ranatrk
Copy link
Contributor

@ranatrk ranatrk commented Dec 20, 2023

Issue weaveworks/weave-gitops-enterprise#3739

  • Add CurrentClusterRef to acd spec to include the current management cluster
  • Update Id of capi to be cluster name instead of uid
  • Add managementClusterRef to capi
    • used as the ClusterID in reconciliation to skip if matches the name of the cluster being checked

Add currentClusterName to acd spec
…r name of cluster to be skipped

Update capi sample acd to include currentClusterRef
@ranatrk ranatrk added the enhancement New feature or request label Dec 20, 2023
@ranatrk ranatrk requested a review from a team December 20, 2023 16:39
Copy link
Collaborator

@bigkevmcd bigkevmcd left a 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"`
Copy link
Collaborator

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"`
}

Copy link
Collaborator

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
Copy link
Collaborator

@bigkevmcd bigkevmcd left a 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)
Copy link
Collaborator

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"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻

@ranatrk ranatrk merged commit 41ed04e into main Dec 21, 2023
3 checks passed
@ranatrk ranatrk deleted the capi-self-filtering branch December 21, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants