-
Notifications
You must be signed in to change notification settings - Fork 420
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
separate SAs for controller/webhook deployment to allow for different permission sets #818
Conversation
/hold cancel ok @dibyom I've confirmed these changes in a new cluster that did not previously have pipelines/triggers interestingly again, my prior commit with this PR did not work as is ... I had to make additional changes as we talked about last time, there definitely does seem to be a clean up issue, most likely I would think around *Role and *RoleBindings, since removing permissions does not seem to get reflected in the CI runs, which were clean on my first push |
/hold cancel ok @dibyom .... tested on a clean cluster with the right, non-overlapping set of role/rolebinding and clusterrole/clusterrolebinding the high level jist:
I'm still curious about |
Hmm, my best guess is that we clean up the namespace but maybe not cluster resources (like cluster roles?). Though I was under the impression that we used fresh clusters for each e2e test....would have to look into it
Sure, will try this out on my setup. |
Tried it out on GKE. Works fine. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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 |
thanks @dibyom |
Fixes #807
Changes
See #807 (comment)
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes
/assign @dibyom
fyi - I have not tried this on a clean installed cluster yet ... will unhold after I do so
/hold