add affirmative output to oc policy / oadm policy

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".
```

Author: juanvallejo

pkg/cmd/admin/policy/modify_roles.go

@@ -43,6 +43,7 @@ type RoleModificationOptions struct {
 	RoleName            string
 	RoleBindingAccessor RoleBindingAccessor
 
+	Targets  []string
 	Users    []string
 	Groups   []string
 	Subjects []kapi.ObjectReference
@@ -63,7 +64,10 @@ func NewCmdAddRoleToGroup(name, fullName string, f *clientcmd.Factory, out io.Wr
 
 			if err := options.AddRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, true, "group", options.Targets, true, out)
+
 		},
 	}
 
@@ -89,7 +93,9 @@ func NewCmdAddRoleToUser(name, fullName string, f *clientcmd.Factory, out io.Wri
 
 			if err := options.AddRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, true, "user", options.Targets, true, out)
 		},
 	}
 
@@ -114,7 +120,9 @@ func NewCmdRemoveRoleFromGroup(name, fullName string, f *clientcmd.Factory, out
 
 			if err := options.RemoveRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, false, "group", options.Targets, true, out)
 		},
 	}
 
@@ -139,7 +147,9 @@ func NewCmdRemoveRoleFromUser(name, fullName string, f *clientcmd.Factory, out i
 
 			if err := options.RemoveRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, false, "user", options.Targets, true, out)
 		},
 	}
 
@@ -164,7 +174,9 @@ func NewCmdAddClusterRoleToGroup(name, fullName string, f *clientcmd.Factory, ou
 
 			if err := options.AddRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, true, "group", options.Targets, false, out)
 		},
 	}
 
@@ -186,7 +198,9 @@ func NewCmdAddClusterRoleToUser(name, fullName string, f *clientcmd.Factory, out
 
 			if err := options.AddRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, true, "user", options.Targets, false, out)
 		},
 	}
 
@@ -208,7 +222,9 @@ func NewCmdRemoveClusterRoleFromGroup(name, fullName string, f *clientcmd.Factor
 
 			if err := options.RemoveRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, false, "group", options.Targets, false, out)
 		},
 	}
 
@@ -230,7 +246,9 @@ func NewCmdRemoveClusterRoleFromUser(name, fullName string, f *clientcmd.Factory
 
 			if err := options.RemoveRole(); err != nil {
 				kcmdutil.CheckErr(err)
+				return
 			}
+			printSuccessForCommand(options.RoleName, false, "user", options.Targets, false, out)
 		},
 	}
 
@@ -247,6 +265,8 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args
 		o.Users = append(o.Users, args[1:]...)
 	}
 
+	o.Targets = o.Users
+
 	if (len(o.Users) == 0) && (len(saNames) == 0) {
 		return errors.New("you must specify at least one user or service account")
 	}
@@ -263,6 +283,7 @@ func (o *RoleModificationOptions) CompleteUserWithSA(f *clientcmd.Factory, args
 	o.RoleBindingAccessor = NewLocalRoleBindingAccessor(roleBindingNamespace, osClient)
 
 	for _, sa := range saNames {
+		o.Targets = append(o.Targets, sa)
 		o.Subjects = append(o.Subjects, kapi.ObjectReference{Namespace: roleBindingNamespace, Name: sa, Kind: "ServiceAccount"})
 	}
 
@@ -277,6 +298,8 @@ func (o *RoleModificationOptions) Complete(f *clientcmd.Factory, args []string,
 	o.RoleName = args[0]
 	*target = append(*target, args[1:]...)
 
+	o.Targets = *target
+
 	osClient, _, _, err := f.Clients()
 	if err != nil {
 		return err
@@ -396,3 +419,25 @@ existingLoop:
 
 	return newSubjects
 }
+
+// prints affirmative output for role modification commands
+func printSuccessForCommand(role string, didAdd bool, targetName string, targets []string, isNamespaced bool, out io.Writer) {
+	preposition := "from"
+	verb := "removed"
+	clusterScope := " cluster "
+	allTargets := fmt.Sprintf("%q", targets)
+	if isNamespaced {
+		clusterScope = " "
+	}
+	if len(targets) > 1 {
+		targetName = fmt.Sprintf("%ss", targetName)
+	} else if len(targets) == 1 {
+		allTargets = fmt.Sprintf("%q", targets[0])
+	}
+	if didAdd {
+		verb = "added"
+		preposition = "to"
+	}
+
+	fmt.Fprintf(out, "Successfully %s the %q%srole %s %s %s.\n", verb, role, clusterScope, preposition, targetName, allTargets)
+}

test/cmd/policy.sh

@@ -30,28 +30,28 @@ os::cmd::expect_failure_and_text 'oc policy add-role-to-user' 'you must specify
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user -z NamespaceWithoutRole' 'you must specify a role'
 os::cmd::expect_failure_and_text 'oc policy add-role-to-user view' 'you must specify at least one user or service account'
 
-os::cmd::expect_success 'oc policy add-role-to-group cluster-admin system:unauthenticated'
-os::cmd::expect_success 'oc policy add-role-to-user cluster-admin system:no-user'
+os::cmd::expect_success_and_text 'oc policy add-role-to-group cluster-admin system:unauthenticated' 'Successfully added the "cluster-admin" role to group "system:unauthenticated"'
+os::cmd::expect_success_and_text 'oc policy add-role-to-user cluster-admin system:no-user' 'Successfully added the "cluster-admin" role to user "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'
 
-os::cmd::expect_success 'oc policy add-role-to-user cluster-admin -z=one,two --serviceaccount=three,four'
+os::cmd::expect_success_and_text 'oc policy add-role-to-user cluster-admin -z=one,two --serviceaccount=three,four' 'Successfully added the "cluster-admin" role to users \["one" "two" "three" "four"\]'
 os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers'
 os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'one'
 os::cmd::expect_success_and_text 'oc get rolebinding/cluster-admin --no-headers' 'four'
 
-os::cmd::expect_success 'oc policy remove-role-from-group cluster-admin system:unauthenticated'
+os::cmd::expect_success_and_text 'oc policy remove-role-from-group cluster-admin system:unauthenticated' 'Successfully removed the "cluster-admin" role from group "system:unauthenticated"'
 
-os::cmd::expect_success 'oc policy remove-role-from-user cluster-admin system:no-user'
-os::cmd::expect_success 'oc policy remove-role-from-user cluster-admin -z=one,two --serviceaccount=three,four'
+os::cmd::expect_success_and_text 'oc policy remove-role-from-user cluster-admin system:no-user' 'Successfully removed the "cluster-admin" role from user "system:no-user"'
+os::cmd::expect_success_and_text 'oc policy remove-role-from-user cluster-admin -z=one,two --serviceaccount=three,four' 'Successfully removed the "cluster-admin" role from users \["one" "two" "three" "four"\]'
 os::cmd::expect_success 'oc get rolebinding/cluster-admin --no-headers'
 os::cmd::expect_success_and_not_text 'oc get rolebinding/cluster-admin --no-headers' 'four'
 
 os::cmd::expect_success 'oc policy remove-group system:unauthenticated'
 os::cmd::expect_success 'oc policy remove-user system:no-user'
 
 
-os::cmd::expect_success 'oc policy add-role-to-user admin namespaced-user'
+os::cmd::expect_success_and_text 'oc policy add-role-to-user admin namespaced-user' 'Successfully added the "admin" role to user "namespaced-user"'
 # Ensure the user has create permissions on builds, but that build strategy permissions are granted through the authenticated users group
 os::cmd::try_until_text              'oadm policy who-can create builds' 'namespaced-user'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/docker' 'namespaced-user'
@@ -62,9 +62,9 @@ os::cmd::expect_success_and_text     'oadm policy who-can create builds/docker'
 os::cmd::expect_success_and_text     'oadm policy who-can create builds/source' 'system:authenticated'
 os::cmd::expect_success_and_text     'oadm policy who-can create builds/jenkinspipeline' 'system:authenticated'
 # if this method for removing access to docker/custom/source/jenkinspipeline builds changes, docs need to be updated as well
-os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-docker system:authenticated'
-os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-source system:authenticated'
-os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-jenkinspipeline system:authenticated'
+os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-docker system:authenticated' 'Successfully removed the "system:build-strategy-docker" cluster role from group "system:authenticated"'
+os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-source system:authenticated' 'Successfully removed the "system:build-strategy-source" cluster role from group "system:authenticated"'
+os::cmd::expect_success_and_text 'oadm policy remove-cluster-role-from-group system:build-strategy-jenkinspipeline system:authenticated' 'Successfully removed the "system:build-strategy-jenkinspipeline" cluster role from group "system:authenticated"'
 # ensure build strategy permissions no longer exist
 os::cmd::try_until_failure           'oadm policy who-can create builds/source | grep system:authenticated'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/docker' 'system:authenticated'
@@ -73,7 +73,7 @@ os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/jenkinsp
 
 # ensure system:authenticated users can not create custom builds by default, but can if explicitly granted access
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/custom' 'system:authenticated'
-os::cmd::expect_success 'oadm policy add-cluster-role-to-group system:build-strategy-custom system:authenticated'
+os::cmd::expect_success_and_text 'oadm policy add-cluster-role-to-group system:build-strategy-custom system:authenticated' 'Successfully added the "system:build-strategy-custom" cluster role to group "system:authenticated"'
 os::cmd::expect_success_and_text 'oadm policy who-can create builds/custom' 'system:authenticated'
 
 os::cmd::expect_success 'oadm policy reconcile-cluster-role-bindings --confirm'