Skip to content

Conversation

simo5
Copy link
Contributor

@simo5 simo5 commented Feb 13, 2018

Fixes #18582

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 13, 2018
@simo5
Copy link
Contributor Author

simo5 commented Feb 13, 2018

@enj PTAL

@simo5
Copy link
Contributor Author

simo5 commented Feb 13, 2018

/retest

@enj
Copy link
Contributor

enj commented Feb 14, 2018

@simo5 can you also add a test like my example from #18102 (comment) (remove-role-from-user).

enj
enj previously requested changes Feb 14, 2018
@@ -54,6 +54,10 @@ os::cmd::expect_success_and_text 'oc policy add-role-to-group cluster-admin --ro
os::cmd::expect_success_and_text 'oc policy add-role-to-user --rolebinding-name cluster-admin cluster-admin system:no-user' 'role "cluster-admin" added: "system:no-user"'
os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers'
os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'system:no-user'
# Test failure to mix and mismatch
os::cmd::expect_success 'oc create rolebinding mismatch --clusterrole=cluster-admin --user=system:user'
os::cmd::expect_failure 'oc policy remove-user --rolebinding-name mismatch system:no-user'
Copy link
Contributor

Choose a reason for hiding this comment

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

os::cmd::expect_failure_and_text with specific failure

@simo5
Copy link
Contributor Author

simo5 commented Feb 14, 2018

I could go the whole project creation route like your example, but it's a ton of test overhead for no additional value.

@simo5
Copy link
Contributor Author

simo5 commented Feb 14, 2018

And of course there is no --rolebinding-name option on this command ... FAIL! :-)

Signed-off-by: Simo Sorce <simo@redhat.com>
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2018
@simo5
Copy link
Contributor Author

simo5 commented Feb 14, 2018

Overhauled the test to use a project and checks for rolebinding mismatches

@simo5 simo5 dismissed enj’s stale review February 14, 2018 17:53

Requested changes applied

@enj
Copy link
Contributor

enj commented Feb 14, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, 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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2018
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 6aba27d into openshift:master Feb 14, 2018
bparees pushed a commit to bparees/origin that referenced this pull request Feb 14, 2018
Automatic merge from submit-queue.

Add tests for remove-user with a rolebinding name

Fixes openshift#18582
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants