Delete (Cluster)Role(Bindings) as a final cleanup step. #359
Conversation
This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in knative#291 is approved, this will become necessary. Additionally, it makes sense to remove roles as the final step to allow human Operators to modify any resources they may have permissions on as a result of the Knative installation (that is, we should not remove any access until we are 'almost done' cleaning up).
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@Cynocracy: 0 warnings.
Details
In response to this:
This allows us to take advantage of the permissions granted to cluster admins
when performing cleanup. If the approach in #291
is approved, this will become necessary. Additionally, it makes sense to remove roles as the final
step to allow human Operators to modify any resources they may have permissions on as a result of the Knative installation (that is, we should not remove any access until we are 'almost done' cleaning up)./lint
Proposed Changes
- Delete roles and rolebindings as a final cleanup step.
Release Note
Delete roles and rolebindings as a final cleanup step.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
/assign @k4leung4 |
|
maybe add a unit test? |
jcrossley3
left a comment
jcrossley3
left a comment
There was a problem hiding this comment.
Without fully grok'ing #291 I'm unable to say what problem this is solving.
|
/lgtm |
| if err := r.config.Filter(mf.NoCRDs, mf.None(RBAC)).Delete(); err != nil { | ||
| return err | ||
| } | ||
| // Delete Roles last, as they may be useful for human operators to clean up. |
There was a problem hiding this comment.
can we add a unit test to verify this behavior
|
@k4leung4 re: unit test, I'm not sure I'd like to expand the scope of this PR to include one. Perhaps I can work on one in a separate issue/change? |
|
@Cynocracy works for me. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cynocracy, k4leung4 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Gotcha, there are integration tests here: Filed #361 to add unit tests. |
5 participants
This allows us to take advantage of the permissions granted to cluster admins
when performing cleanup. If the approach in #291
is approved, this will become necessary. Additionally, it makes sense to remove roles as the final
step to allow human Operators to modify any resources they may have permissions on as a result of the Knative installation (that is, we should not remove any access until we are 'almost done' cleaning up).
/lint
Proposed Changes
Release Note