Update namespace finalizer to delete RoleBindingRestrictions #13563
Conversation
| core.NewListAction(unversioned.GroupVersionResource{Group: "", Version: "v1", Resource: "deploymentconfig"}, "", kapi.ListOptions{}), | ||
| core.NewListAction(unversioned.GroupVersionResource{Group: "", Version: "v1", Resource: "egressnetworkpolicy"}, "", kapi.ListOptions{}), | ||
| namespaceFinalize, | ||
| core.NewListAction(unversioned.GroupVersionResource{Group: "", Version: "", Resource: "buildconfigs"}, testNamespace.Name, kapi.ListOptions{}), |
There was a problem hiding this comment.
The fake clients only keep track of resource, they always pass the empty string for group and version.
There was a problem hiding this comment.
This isn't obvious. Is there a smaller change for a backport? Especially on a test for a controller we're going to remove post-rebase.
There was a problem hiding this comment.
The old test did not check anything but the total number of actions (i.e. it was useless) so we have not lost anything.
There was a problem hiding this comment.
The old test did not check anything but the total number of actions (i.e. it was useless) so we have not lost anything.
If it was useless, then you needed no changes to pass, right? I'd sort of expect to see some change here even if you didn't have to do all this to have equivalence.
There was a problem hiding this comment.
Yes, at the bare minimum you had to copy paste one of the NewListAction lines.
There was a problem hiding this comment.
Yes, at the bare minimum you had to copy paste one of the NewListAction lines.
For code that we know we're going to delete, that has a fixed point where we're going to delete it, that we know we're going to backport, I'm not sure I'd make the larger change.
@mfojtik have you reasoned through the test to see if it still works? I haven't, but if we really want to go to the effort and we know this change is ok then actually checking is clearly better.
|
lgtm |
Signed-off-by: Monis Khan <[email protected]>
enj
force-pushed
the
enj/i/rbr_reaper/1435132
branch
from
335d786 to
4e5f007
Compare
|
Evaluated for origin test up to 4e5f007 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/496/) (Base Commit: 2130ec6) |
|
lgtm [merge] |
|
Evaluated for origin merge up to 4e5f007 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/220/) (Base Commit: 18962de) (Image: devenv-rhel7_6108) |
…3563-upstream-ose-enterprise-3.5 Merged by openshift-bot
https://bugzilla.redhat.com/show_bug.cgi?id=1435132
[test]
Signed-off-by: Monis Khan [email protected]
Who wants to review 😄
cc @ncdc @mfojtik @derekwaynecarr @deads2k @liggitt @smarterclayton