-
Notifications
You must be signed in to change notification settings - Fork 382
Update rbac.yaml to conditionally create namespace scoped resource RBAC #2194
Update rbac.yaml to conditionally create namespace scoped resource RBAC #2194
Conversation
@@ -106,6 +106,20 @@ items: | |||
- apiGroups: ["servicecatalog.k8s.io"] | |||
resources: ["clusterservicebrokers/status","clusterserviceclasses/status","clusterserviceplans/status","serviceinstances/status","serviceinstances/reference","servicebindings/status"] | |||
verbs: ["update"] | |||
{{- if .Values.namespacedServiceBrokerEnabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we remove the gate, we'll need to remove this as well.
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once the changes to Chart.yaml are backed out.
charts/catalog/Chart.yaml
Outdated
@@ -1,3 +1,4 @@ | |||
name: catalog | |||
description: service-catalog API server and controller-manager helm chart | |||
version: 0.1.24 | |||
version: 0.1.25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bumped manually during our releases. You don't need to touch it here.
See https://github.com/kubernetes-incubator/service-catalog/pull/2192/files for what a release looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think i'll open another issue to allow these to be changed independently. As is, the chart leaves svc cat in a broken state of someone turns on the feature gate, so we should probably do another release (which makes me a little sad)
…catalog into helm-chart-update
New changes are detected. LGTM label has been removed. |
@carolynvs removed the change to Chart.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@eriknelson Are you able to add the LGTM2 label and merge once the tests pass? If not, let me know so that we can fix your perms. We aren't using |
@carolynvs woops, yeah I'll add the label manually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine. the duplication irritates me. not sure what to do about it. I don't want to mess with go templates and variables. probably better to be explicit and hope it stays in sync.
/approve
had one more thing to add, but I forgot.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MHBauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Think we have a consensus. Merging! |
Update rbac.yaml to conditionally create namespace scoped resource RBAC rules. Also adds "appVersion" to the helm chart (we can keep this at the version) so we can update the actual helm chart version (i.e. to fix bugs like this)
Closes: #2193