Closed Cynocracy closed 4 years ago
/assign @k4leung4
maybe add a unit test?
/lgtm
@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. just want to make sure we verify behavior changes as we dont really know when it stops working.
/approve /lgtm
[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
Gotcha, there are integration tests here: https://github.com/knative/serving-operator/blob/master/test/resources/verify.go#L190
Filed https://github.com/knative/serving-operator/issues/361 to add unit tests.
This allows us to take advantage of the permissions granted to cluster admins when performing cleanup. If the approach in https://github.com/knative/serving-operator/pull/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