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

fix: remove duplicated labels on rbac components #61

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

rafaribe
Copy link
Contributor

@rafaribe rafaribe commented Feb 28, 2023

I was trying to use your operator with FluxCD and noticed that the RBAC components already have the following labels defined in cloudflare-zero-trust-operator.labels.

  • app.kubernetes.io/instance
  • app.kubernetes.io/managed-by
  • app.kubernetes.io/name

Therefore when the chart is rendered, there's duplication of labels.
This makes it impossible to use with FluxCD due to #283.

Thank you in advance!

@jzweifel
Copy link

❤️

@BojanZelic
Copy link
Owner

Thanks for the contribution! 🎉

Couple of points:

First, The files in helm/cloudflare-zero-trust-operator/templates are auto-generated based on the kustomize manifests, so if I merge this as-is it won't work, since it will just be overwritten; operator-sdk generates the rbac kustomize manifests based off kubebuilder annotations so I'd like to keep the single source of truth;

take a look at https://github.com/BojanZelic/cloudflare-zero-trust-operator/blob/main/Makefile#L213-L221 to see how the helm template is created; We could possibly use yq or sed (or if you have any other ideas) here in the makefile to remove the labels;

you should be able to make helm locally to verify it; This command gets ran every time a release is made;

Second, there's no need to update the chart version; it's automatically handled when we tag & build a release: https://github.com/BojanZelic/cloudflare-zero-trust-operator/blob/main/.github/workflows/perform-release.yaml#L31

Let me know how it goes! I can also take a look later in the week if there are any issues;

@rafaribe
Copy link
Contributor Author

rafaribe commented Mar 1, 2023

I've pushed a new commit that should address the things you mentioned. I force-pushed it to not clutter the main repo with the previous approach.
Take a look when you can and let me know how it goes :)

@BojanZelic BojanZelic merged commit 7bff14d into BojanZelic:main Mar 1, 2023
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