Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add affirmative output to oc policy / oadm policy #12324

Merged
merged 1 commit into from
Jan 16, 2017
Merged

add affirmative output to oc policy / oadm policy #12324

merged 1 commit into from
Jan 16, 2017

Conversation

juanvallejo
Copy link
Contributor

fixes: #12284

This patch adds affirmative output to commands that modify cluster and
local roles for users and groups.

Examples

$ oadm policy remove-cluster-role-from-group self-provisioner
system:authenticated:oauth system:authenticated
Successfully removed the "self-provisioner" cluster role from groups
["system:authenticated:oauth" "system:authenticated"].

$ oadm policy add-cluster-role-to-group self-provisioner
system:authenticated:oauth
Successfully added the "self-provisioner" cluster role to group
"system:authenticated:oauth".

$ oc policy add-role-to-user cluster-admin testuser
Successfully added the "cluster-admin" role to user "testuser".

$ oc policy remove-role-from-user cluster-admin testuser
Successfully removed the "cluster-admin" role from user "testuser".

@openshift/cli-review

@juanvallejo
Copy link
Contributor Author

[test]

@stevekuznetsov
Copy link
Contributor

Can probably ditch the "successfully" prefix.

@juanvallejo
Copy link
Contributor Author

@stevekuznetsov

Can probably ditch the "successfully" prefix.

Hm, it sounds a bit off if it outputs Added the "self-provisioner" cluster role to group "system:authenticated:oauth". Maybe update the sentence so it reads something like The "self-provisioner" cluster role has been added to the group "system:authenticated:oauth"?

@juanvallejo
Copy link
Contributor Author

conformance check flaked on #11662 re[test]

@jerboaa jerboaa mentioned this pull request Dec 22, 2016
Merged
@fabianofranz
Copy link
Member

@liggitt PTAL

liggitt
liggitt reviewed Dec 22, 2016
View reviewed changes
Copy link
Contributor

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

Agree it seems a little verbose

pkg/cmd/admin/policy/modify_roles.go

// prints affirmative output for role modification commands
func printSuccessForCommand(role string, didAdd bool, targetName string, targets []string, isNamespaced bool, out io.Writer) {
preposition := "from"
Copy link
Contributor

Choose a reason for hiding this comment

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

This string building isn't going to play nice with translation efforts

@juanvallejo
Copy link
Contributor Author

@liggitt @stevekuznetsov shortened the output to read:

$ oadm policy remove-cluster-role-from-group self-provisioner system:authenticated:oauth system:authenticated
cluster role "self-provisioner" removed: ["system:authenticated:oauth" "system:authenticated"]

$ oadm policy add-cluster-role-to-group self-provisioner system:authenticated:oauth
cluster role "self-provisioner" added: "system:authenticated:oauth"

will update tests if this reads okay

@stevekuznetsov
Copy link
Contributor

Sounds fine, it mirrors the output from oc create today. We could render lists with a comma-delimited string to make them a little more friendly, but this is an administrative command so it's not as important as it would be on oc new-app. I'll defer to @openshift/cli-review.

@juanvallejo juanvallejo force-pushed the jvallejo/add-affirmative-output-policy-cmds branch from f8e1faa to 517bb93 Compare January 9, 2017 21:12
@juanvallejo
Copy link
Contributor Author

@stevekuznetsov Thanks for the feedback, will leave slices with []. Updated tests to match new output

@juanvallejo
Copy link
Contributor Author

@fabianofranz ptal

@fabianofranz
Copy link
Member

LGTM, holding the merge until rebase lands.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/add-affirmative-output-policy-cmds branch from 517bb93 to ef01748 Compare January 11, 2017 15:07
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017
@fabianofranz
Copy link
Member

[merge]

@juanvallejo
Copy link
Contributor Author

merge check flaked on #11024 re[test]

@juanvallejo
Copy link
Contributor Author

conformance flaked on #12184 re[test]

@fabianofranz
Copy link
Member

re[merge]

@juanvallejo juanvallejo force-pushed the jvallejo/add-affirmative-output-policy-cmds branch from ef01748 to 303f7e7 Compare January 12, 2017 21:26
@juanvallejo
add affirmative output to oc policy / oadm policy
6009e22 
This patch adds affirmative output to commands that modify cluster and
local roles for users and groups.

**Examples**
```
$ oadm policy remove-cluster-role-from-group self-provisioner
system:authenticated:oauth system:authenticated
Successfully removed the "self-provisioner" cluster role from groups
["system:authenticated:oauth" "system:authenticated"].

$ oadm policy add-cluster-role-to-group self-provisioner
system:authenticated:oauth
Successfully added the "self-provisioner" cluster role to group
"system:authenticated:oauth".

$ oc policy add-role-to-user cluster-admin testuser
Successfully added the "cluster-admin" role to user "testuser".

$ oc policy remove-role-from-user cluster-admin testuser
Successfully removed the "cluster-admin" role from user "testuser".
```
@juanvallejo juanvallejo force-pushed the jvallejo/add-affirmative-output-policy-cmds branch from 303f7e7 to 6009e22 Compare January 13, 2017 14:36
@juanvallejo
Copy link
Contributor Author

test flaked on #11887 re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6009e22

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12849/) (Base Commit: 398ca1a)

@fabianofranz
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 16, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12849/) (Image: devenv-rhel7_5690)

@fabianofranz
Copy link
Member

flaked on #11016 re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6009e22

@openshift-bot openshift-bot merged commit 54becc6 into openshift:master Jan 16, 2017
@juanvallejo juanvallejo deleted the jvallejo/add-affirmative-output-policy-cmds branch January 16, 2017 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

oadm policy commands don't have affirmative output
5 participants
@juanvallejo @stevekuznetsov @fabianofranz @openshift-bot @liggitt