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

feat: Custom labels and annotations for namespace. #702

Conversation

cpitstick-latai
Copy link
Contributor

@cpitstick-latai cpitstick-latai commented Jul 30, 2024

This PR

Allows custom labels and annotations to be added to a namespace. It also makes it possible to simply disable namespace creation for clusters where ownership of the namespace needs to lie elsewhere for policy or structural reasons.

Related Issues

Re-implements #678

Notes

Note 1

Fully rendered namespace.yaml after running make helm-package (Extracted from /charts/open-feature-operator-v0.7.1.tgz using tar -xvf open-feature-operator-v0.7.1.tgz)

# Only deploy the namespace if the default is being used (helm install should fail if the namespace isnt present)
# when one is defined with -n
{{- if and (eq (include "chart.namespace" .) .Values.defaultNamespace) .Values.namespace.create }}
apiVersion: v1
kind: Namespace
metadata:
  name: '{{ include "chart.namespace" . }}'
  labels:
    control-plane: controller-manager
    {{- range $key, $value := $.Values.namespace.labels }}
    {{ $key }}: {{ $value | quote }}
    {{- end }}
  annotations:
    {{- range $key, $value := $.Values.namespace.annotations }}
    {{ $key }}: {{ $value | quote }}
    {{- end }}
{{ end }}

Note 2

This does not need to utilize any of the special directives of strip-kustomize-helm.sh because it is not referenced in the helm overlay kustomization

Note 3

I side-stepped the issue that #678 has with the confusion between namespace.name and defaultNamespace by simply leaving defaultNamespace in place and not adding another name field. It is slightly awkward because now we have a namespace section and a standalone defaultNamespace variable. However, I don't believe it's worth breaking backwards compatibility over this.

Follow-up Tasks

How to test

  1. Add labels and annotations to the values.
  2. Deploy the helm chart with namespace.create: true set
  3. Validate that the labels and annotations appear on the namespace.
  4. Set defaultNamespace to null and test with create to be true/false (if false, create namespace externally from the helm chart)

@cpitstick-latai cpitstick-latai force-pushed the cpitstick-latai/labels-annotations-namespace branch from 83049e3 to 50e68d5 Compare July 30, 2024 17:45
@cpitstick-latai cpitstick-latai marked this pull request as ready for review July 30, 2024 18:05
@cpitstick-latai cpitstick-latai requested a review from a team as a code owner July 30, 2024 18:05
@cpitstick-latai cpitstick-latai force-pushed the cpitstick-latai/labels-annotations-namespace branch from 50e68d5 to c570c02 Compare July 30, 2024 18:23
@cpitstick-latai
Copy link
Contributor Author

cpitstick-latai commented Jul 31, 2024

I have tested adding custom labels and annotations with create: true and defaultNamespace: open-feature-operator-system, both custom labels and annotations were added properly to the namespace as it was created.

@cpitstick-latai
Copy link
Contributor Author

I have validated that this fixes #683 if namespace.create: false is set

You could say, well "what about if it's null and namespace.create is true?" That scenario doesn't make a ton of sense to me, so I think it's worth skipping it.

Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan left a comment

Choose a reason for hiding this comment

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

looks good from my end :)

Also makes it possible to simply disable namespace creation for clusters
where ownership of the namespace needs to lie elsewhere for policy or
structural reasons.

Signed-off-by: Christopher Pitstick <cpitstick@lat.ai>
@Kavindu-Dodan Kavindu-Dodan force-pushed the cpitstick-latai/labels-annotations-namespace branch from c570c02 to b1f7666 Compare August 2, 2024 17:42
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.05%. Comparing base (499661e) to head (b1f7666).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   86.51%   87.05%   +0.53%     
==========================================
  Files          19       19              
  Lines        1587     1321     -266     
==========================================
- Hits         1373     1150     -223     
+ Misses        173      128      -45     
- Partials       41       43       +2     

see 18 files with indirect coverage changes

Flag Coverage Δ
unit-tests 87.05% <ø> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@beeme1mr beeme1mr merged commit a21f278 into open-feature:main Aug 2, 2024
18 checks passed
@github-actions github-actions bot mentioned this pull request Aug 2, 2024
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.

3 participants