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

Adding namespaceLabels and namespaceAnnotations as TargetCustomization #2583

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

Tommy12789
Copy link
Contributor

@Tommy12789 Tommy12789 commented Jul 3, 2024

Refers to #2262 #2441

Added a new parameter namespaceLabels in bundle targets to customize bundleDeployment with those labels

@Tommy12789 Tommy12789 force-pushed the 2262 branch 5 times, most recently from 1998d0f to ed555da Compare July 9, 2024 13:48
@Tommy12789 Tommy12789 changed the title Adding namespaceLabels as TargetCustomization Adding namespaceLabels and namespaceAnnotations as TargetCustomization Jul 9, 2024
@Tommy12789 Tommy12789 marked this pull request as ready for review July 9, 2024 13:49
@Tommy12789 Tommy12789 requested a review from a team as a code owner July 9, 2024 13:49
@@ -221,6 +221,12 @@ type BundleTarget struct {
ClusterGroupSelector *metav1.LabelSelector `json:"clusterGroupSelector,omitempty"`
// DoNotDeploy if set to true, will not deploy to this target.
DoNotDeploy bool `json:"doNotDeploy,omitempty"`
// NamespaceLabels are labels that will be appended to the namespace created by Fleet.
// +nullable
NamespaceLabels *map[string]string `json:"namespaceLabels,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I see this is already a pointer to a map in the bundledeployment type. Is that really necessary? Looking at corev1 in k8s, it seems +optional and omitempty is sufficient, unless we really need to differentiate between a null value and an empty map.

Oh, it seems we do:

if bd.Spec.Options.NamespaceAnnotations != nil {

That code deletes all annotations if it's an empty map, but not if it's nil. 👍

Copy link
Member

Choose a reason for hiding this comment

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

However that would still work with a map, as a map is effectively a pointer.

@manno manno merged commit eba7879 into rancher:main Jul 22, 2024
12 checks 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