Restore delegated_test.go #16180
Restore delegated_test.go #16180
Conversation
openshift-ci-robot
added
the
size/M
mrogers950
force-pushed
the
delegated_test
branch
from
e3c258b
to
c58579a
Compare
|
@deads2k do you remember the purpose of that old test? |
mrogers950
force-pushed
the
delegated_test
branch
from
c58579a
to
6bf7af4
Compare
That test made sure we waited for the rolebinding before returning so that clients could immediately access resources in the project. |
mrogers950
force-pushed
the
delegated_test
branch
from
6bf7af4
to
32c253b
Compare
|
@enj I've fixed the restored version and rebased. |
|
/retest |
mrogers950
force-pushed
the
delegated_test
branch
from
32c253b
to
3acb897
Compare
|
/retest |
|
@enj does this need anything else? |
|
/approve Delegating review to @simo5 @php-coder @adelton |
| if t.bindings[bootstrappolicy.AdminRoleName] != nil { | ||
| return t.bindings[bootstrappolicy.AdminRoleName], nil | ||
| } | ||
| return nil, errors.NotFound("") |
There was a problem hiding this comment.
Perhaps, it's better to use non-empty error message. If we were searching for bootstrappolicy.AdminRoleName, we can include that in the message.
|
|
||
| select { | ||
| case <-callReturnedCh: | ||
| t.Errorf("too fast, should have blocked") |
There was a problem hiding this comment.
I'd use t.Error() here and rephrase the error message to provide more details (like what happens, what is expected, waiting time).
I just imagined myself, if I would found too fast, should have blocked error somewhere in the long test execution log, then I'd like to have more details.
| "github.com/openshift/origin/pkg/cmd/server/bootstrappolicy" | ||
| ) | ||
|
|
||
| func TestDelegatedWait(t *testing.T) { |
There was a problem hiding this comment.
I'd suggest to split this to 2 functions.
There was a problem hiding this comment.
P.S. Unless I miss something. Do we really need to check 2 cases sequentially within one test method?
There was a problem hiding this comment.
Yeah, to test the unblock we want to ensure it blocked in the first place, so it makes sense to me to have this sequence in one test.
| cache.namespacelister.lock.Lock() | ||
| defer cache.namespacelister.lock.Unlock() | ||
| cache.namespacelister.bindings[bootstrappolicy.AdminRoleName] = &rbac.RoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: bootstrappolicy.AdminRoleName}, |
There was a problem hiding this comment.
Is it possible to extract this to a function in the testRoleBindingLister struct?
| } | ||
| } | ||
|
|
||
| func testWait(storage *REST) chan struct{} { |
There was a problem hiding this comment.
testWait() is to generic name and tells me nothing. Consider to rename it to something more meaningful, for example, waitForAdminRoleInTheStorage().
mrogers950
force-pushed
the
delegated_test
branch
from
3acb897
to
091a2cc
Compare
|
@php-coder rebased and updated |
|
/approve |
openshift-ci-robot
added
the
approved
|
flake #17882 |
|
/test extended_conformance_install |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, mrogers950, simo5 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 18163, 18139, 18193, 18194, 16180). |
delegated_test.go covered waitForRoleBinding() and was removed in c96408a. This updates it to use native RBAC objects.
One of the fixes for #15836