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: add namespace selector using env variable #1434

Merged

Conversation

atos-cosmin-gavagiuc
Copy link
Contributor

add namespace selector 1433

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2024

CLA assistant check
All committers have signed the CLA.

@HVBE
Copy link
Collaborator

HVBE commented Feb 21, 2024

@NissesSenap @weisdd @pb82 @theSuess thoughts? IMHO doesn't harm to have another way to specify this

@NissesSenap
Copy link
Collaborator

Sounds reasonable to me.
Before merging, it would be nice if we can add this feature to the helm chart

How do we see this working with RBAC, is it up to the grafana-operator admins to fix this on their own?
I'm fine with that, but it would be nice to document.

It would also be nice if we added an e2e test for this.
It's probably something that @eddycharly could help with. It would be similar to #1437, but I'm unsure how we would make the operator be installed in different modes in the best way.
One for all the existing tests and this one using the new namespace label selector.

@eddycharly
Copy link
Contributor

@NissesSenap i'm lacking context here, what is this supposed to achieve ?

@NissesSenap
Copy link
Collaborator

@eddycharly running the operator in the new mode called namespace_selector.
Using a label on the namespace, and depending on that applying dashboard etc to grafana instances.

I guess we need to run another instance of the operator. Create 2 namespaces, one with the label and one without and see what only one of the dashboards gets picked up by the grafana instances. Or something like that.

Writing on my phone before going to bed, so sorry if I didn't think everything through.

@NissesSenap
Copy link
Collaborator

NissesSenap commented Mar 12, 2024

@atos-cosmin-gavagiuc , can you look at the comments in #1434 (comment)? It won't get merged until some kind of documentation exist.

@atos-cosmin-gavagiuc
Copy link
Contributor Author

atos-cosmin-gavagiuc commented Mar 12, 2024

@atos-cosmin-gavagiuc , can you look at the comments in #1434 (comment)? It won't get merged until some kind of documentation exist.

@NissesSenap, I've updated the helm chart 6699ee2. I'm not familiar with helm and I might have done it wrong.

@NissesSenap
Copy link
Collaborator

I update the helm config so it works with a string like environment: dev

We will keep RBAC in helm to cluster level access, since we can't force which namespace will need the access, since it's based on labels.

Please write some docs about this new feature here: https://github.com/grafana/grafana-operator/blob/master/docs/docs/grafana.md#where-should-the-operator-look-for-grafana-resources
and we should be able to merge.

Thanks for your work @atos-cosmin-gavagiuc .

@atos-cosmin-gavagiuc
Copy link
Contributor Author

@NissesSenap I've added the a short description in the docs 510a9b7f6fd742f6f685489deab8f1e0d517ce25

Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

LGTM

@atos-cosmin-gavagiuc
Copy link
Contributor Author

@NissesSenap I've fixed that trailing whitespace.

@NissesSenap NissesSenap merged commit 81aafca into grafana:master Mar 14, 2024
10 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.

None yet

5 participants