Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Delete (Cluster)Role(Bindings) as a final cleanup step. #141

Merged
merged 3 commits into from
Mar 16, 2020

Conversation

Cynocracy
Copy link
Contributor

This allows us to take advantage of the permissions granted to cluster admins
when performing cleanup. If the approach in #109
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.

Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@Cynocracy: 0 warnings.

In response to this:

This allows us to take advantage of the permissions granted to cluster admins
when performing cleanup. If the approach in #109
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.

config/role.yaml Outdated
@@ -26,9 +26,6 @@ rules:
- apps
resources:
- deployments
- daemonsets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, this shouldn't be here, one moment..

This allows us to take advantage of the permissions granted to cluster admins
when performing cleanup. If the approach in knative#109
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).
@Cynocracy
Copy link
Contributor Author

Whoops, looks like I need to update manifestival, one moment.

@Cynocracy Cynocracy changed the title Delete (Cluster)Role(Bindings) as a final cleanup step. [WIP] Delete (Cluster)Role(Bindings) as a final cleanup step. Mar 13, 2020
@Cynocracy
Copy link
Contributor Author

Blocked on #142

@k4leung4
Copy link
Contributor

is this still WIP?

@Cynocracy Cynocracy changed the title [WIP] Delete (Cluster)Role(Bindings) as a final cleanup step. Delete (Cluster)Role(Bindings) as a final cleanup step. Mar 13, 2020
@Cynocracy
Copy link
Contributor Author

Cynocracy commented Mar 13, 2020

/assign @matzew

See also knative/serving-operator#359

@Cynocracy
Copy link
Contributor Author

Cynocracy commented Mar 13, 2020

Oh, sorry k4leung4, missed your comment, no it is no longer WIP.

@Cynocracy
Copy link
Contributor Author

@googlebot I consent

@k4leung4
Copy link
Contributor

/approve
/lgtm

@knative-prow-robot
Copy link
Contributor

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 3f0fc10 into knative:master Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants