-
Notifications
You must be signed in to change notification settings - Fork 91
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
clusterrole.yaml: add if condition for .Values.rbac.enabled #242
clusterrole.yaml: add if condition for .Values.rbac.enabled #242
Conversation
Hi @kastl-ars Question is do we also want to have Otherwise the Issue raised and PR is really helpful |
…ues.rbac.enabled, like on the clusterrolebinding fix opencost#241
1918c7c
to
2228329
Compare
Sorry, I am not quite sure what you mean?
In my opinion, as the OpenShift stuff needs to be enabled explicitly and there is already a value to enable/disable the creation of the clusterRoleBinding, I would say they can be left independent of each other. https://github.com/opencost/opencost-helm-chart/blob/main/charts/opencost/values.yaml#L340 The cluster I want to run OpenCost in does not allow me to create roles and rolebindings, so I will keep these settings disabled and have the cluster owner create them for me. But I will enable the general OpenShift integration. |
Is there anything from blocking this from being merged and released? This prevents me from running OpenCost in one of my clusters, where my permissions do not allow creating roles and rolebindings. And the fix is simple... |
Could someone have a look and merge this, please? It's really simple... |
This will need a version bump. Otherwise all fine |
@mittal-ishaan you want to bump that rq and I'll approve and merge? |
I am not sure if I might be able to add a commit in the PR as it is a fork. Should I go ahead creating a seperate PR with this change and version bump to get this in quickly? |
Hi all, I just pushed a commit that changes the chart version to 1.43.1 (or should this be 1.44.0?). Anything else missing? Not sure about the |
version bump is fine. I think the last |
…nside if condition
I just pushed a commit that moves it into the if condition. Or should I remove it completely? |
clusterrole.yaml: add if condition for .Values.rbac.enabled, like on the clusterrolebinding
fix #241