-
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
Provide ability to add labels to Cluster
object
#809
base: main
Are you sure you want to change the base?
Conversation
805f843
to
f080795
Compare
f080795
to
3475d9c
Compare
3475d9c
to
44b336c
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.
lgtm, 2 charts-related-mostly-nit comments
651f907
to
b5a7452
Compare
b5a7452
to
1f07334
Compare
|
||
if _, ok := values["clusterLabels"]; !ok { | ||
// Use the ManagedCluster's own labels if not defined. | ||
values["clusterLabels"] = mc.GetObjectMeta().GetLabels() |
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.
It doesn't seem reliable since there's no guarantee that all cluster templates have this field (unless you want to make this field required).
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.
Yes, in this approach we are depending on the helm chart for cluster templates to use the clusterLabels
value. We do need some way to assign labels to Cluster
objects. Maybe the better approach would be?
- In the HMC controller, use the values defined in
clusterLabels
in the ManagedCluster spec and patchCluster
to apply those labels to it. - Or if
clusterLabels
is not defined, then use the ManagedCluster's own labels.
values["clusterIdentity"] = cred.Spec.IdentityRef | ||
|
||
if _, ok := values["clusterLabels"]; !ok { | ||
// Use the ManagedCluster's own labels if not defined. |
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.
My understanding was that clusterSelector
in MultiClusterService
is supposed to refer hmc ManagedCluster
instead of capi Cluster
objects. In that case propagation should not be optional. Is my assumption valid?
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.
Right now the ClusterSelector metav1.LabelSelector
field is fed directly into the the .spec.clusterSelector
field for the Sveltos ClusterProfile object. So the label selectors are matching Cluster
objects.
1f07334
to
9396b6e
Compare
Description
Provide ability to add labels to the
Cluster
object created when installing a clusterTemplate by:.Values.ClusterLabels
to all clusterTemplate helm charts.ManagedCluster
's own labels via the HMC controller if none have been defined in.Values.ClusterLabels
.Testing
As can be seen from the output below, the
.metadata.labels
fromManagedCluster
have been added to theCluster
object: