Modify error handling for role and group commands

if command failed, we usually raise exception,
if command success, sometimes there is not any
output (such as set, add commands)
So modify the error handling for role and group
commands.

Change-Id: I1c0f86c04dcedd9c0d725fd73f3436be9da75ee0

Author: Huanxuan Ao

openstackclient/identity/v3/group.py

@@ -62,18 +62,13 @@ def take_action(self, parsed_args):
 
         try:
             identity_client.users.add_to_group(user_id, group_id)
-        except Exception:
-            msg = _("%(user)s not added to group %(group)s\n") % {
-                'user': parsed_args.user,
-                'group': parsed_args.group,
-            }
-            sys.stderr.write(msg)
-        else:
-            msg = _("%(user)s added to group %(group)s\n") % {
+        except Exception as e:
+            msg = _("%(user)s not added to group %(group)s: %(e)s") % {
                 'user': parsed_args.user,
                 'group': parsed_args.group,
+                'e': e,
             }
-            sys.stdout.write(msg)
+            raise exceptions.CommandError(msg)
 
 
 class CheckUserInGroup(command.Command):
@@ -306,18 +301,13 @@ def take_action(self, parsed_args):
 
         try:
             identity_client.users.remove_from_group(user_id, group_id)
-        except Exception:
-            msg = _("%(user)s not removed from group %(group)s\n") % {
-                'user': parsed_args.user,
-                'group': parsed_args.group,
-            }
-            sys.stderr.write(msg)
-        else:
-            msg = _("%(user)s removed from group %(group)s\n") % {
+        except Exception as e:
+            msg = _("%(user)s not removed from group %(group)s: %(e)s") % {
                 'user': parsed_args.user,
                 'group': parsed_args.group,
+                'e': e,
             }
-            sys.stdout.write(msg)
+            raise exceptions.CommandError(msg)
 
 
 class SetGroup(command.Command):

openstackclient/identity/v3/role.py

@@ -16,7 +16,6 @@
 """Identity v3 Role action implementations"""
 
 import logging
-import sys
 
 from keystoneauth1 import exceptions as ks_exc
 from osc_lib.command import command
@@ -129,7 +128,9 @@ def take_action(self, parsed_args):
 
         if (not parsed_args.user and not parsed_args.domain
                 and not parsed_args.group and not parsed_args.project):
-            return
+            msg = _("Role not added, incorrect set of arguments "
+                    "provided. See openstack --help for more details")
+            raise exceptions.CommandError(msg)
 
         domain_id = None
         if parsed_args.role_domain:
@@ -143,11 +144,6 @@ def take_action(self, parsed_args):
 
         kwargs = _process_identity_and_resource_options(
             parsed_args, self.app.client_manager.identity)
-        if not kwargs:
-            sys.stderr.write(_("Role not added, incorrect set of arguments "
-                               "provided. See openstack --help for more "
-                               "details\n"))
-            return
 
         identity_client.roles.grant(role.id, **kwargs)
 
@@ -372,10 +368,10 @@ def take_action(self, parsed_args):
                                '<group-name> --project <project-name> --names '
                                'instead.'))
         else:
-            sys.stderr.write(_("Error: If a user or group is specified, "
-                               "either --domain or --project must also be "
-                               "specified to list role grants.\n"))
-            return ([], [])
+            msg = _("Error: If a user or group is specified, "
+                    "either --domain or --project must also be "
+                    "specified to list role grants.")
+            raise exceptions.CommandError(msg)
 
         return (columns,
                 (utils.get_item_properties(
@@ -405,9 +401,9 @@ def take_action(self, parsed_args):
 
         if (not parsed_args.user and not parsed_args.domain
                 and not parsed_args.group and not parsed_args.project):
-            sys.stderr.write(_("Incorrect set of arguments provided. "
-                               "See openstack --help for more details\n"))
-            return
+            msg = _("Incorrect set of arguments provided. "
+                    "See openstack --help for more details")
+            raise exceptions.CommandError(msg)
 
         domain_id = None
         if parsed_args.role_domain:
@@ -421,11 +417,6 @@ def take_action(self, parsed_args):
 
         kwargs = _process_identity_and_resource_options(
             parsed_args, self.app.client_manager.identity)
-        if not kwargs:
-            sys.stderr.write(_("Role not removed, incorrect set of arguments "
-                               "provided. See openstack --help for more "
-                               "details\n"))
-            return
         identity_client.roles.revoke(role.id, **kwargs)
 
 

openstackclient/tests/functional/identity/v3/test_group.py

@@ -102,11 +102,7 @@ def test_group_add_user(self):
                                     'user_domain': self.domain_name,
                                     'group': group_name,
                                     'user': username})
-        self.assertEqual(
-            '%(user)s added to group %(group)s\n' % {'user': username,
-                                                     'group': group_name},
-            raw_output
-        )
+        self.assertOutput('', raw_output)
 
     def test_group_contains_user(self):
         group_name = self._create_dummy_group()
@@ -128,11 +124,7 @@ def test_group_contains_user(self):
                                     'user_domain': self.domain_name,
                                     'group': group_name,
                                     'user': username})
-        self.assertEqual(
-            '%(user)s added to group %(group)s\n' % {'user': username,
-                                                     'group': group_name},
-            raw_output
-        )
+        self.assertOutput('', raw_output)
         raw_output = self.openstack(
             'group contains user '
             '--group-domain %(group_domain)s '
@@ -165,14 +157,5 @@ def test_group_remove_user(self):
                                     'user_domain': self.domain_name,
                                     'group': group_name,
                                     'user': username})
-        self.assertEqual(
-            '%(user)s added to group %(group)s\n' % {'user': username,
-                                                     'group': group_name},
-            add_raw_output
-        )
-        self.assertEqual(
-            '%(user)s removed from '
-            'group %(group)s\n' % {'user': username,
-                                   'group': group_name},
-            remove_raw_output
-        )
+        self.assertOutput('', add_raw_output)
+        self.assertOutput('', remove_raw_output)

openstackclient/tests/unit/identity/v3/test_group.py

@@ -70,6 +70,20 @@ def test_group_add_user(self):
             self.user.id, self.group.id)
         self.assertIsNone(result)
 
+    def test_group_add_user_with_error(self):
+        self.users_mock.add_to_group.side_effect = exceptions.CommandError()
+        arglist = [
+            self.group.name,
+            self.user.name,
+        ]
+        verifylist = [
+            ('group', self.group.name),
+            ('user', self.user.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+
 
 class TestGroupCheckUser(TestGroup):
 
@@ -460,6 +474,21 @@ def test_group_remove_user(self):
             self.user.id, self.group.id)
         self.assertIsNone(result)
 
+    def test_group_remove_user_with_error(self):
+        self.users_mock.remove_from_group.side_effect = (
+            exceptions.CommandError())
+        arglist = [
+            self.group.id,
+            self.user.id,
+        ]
+        verifylist = [
+            ('group', self.group.id),
+            ('user', self.user.id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+
 
 class TestGroupSet(TestGroup):
 

openstackclient/tests/unit/identity/v3/test_role.py

@@ -273,6 +273,22 @@ def test_role_add_domain_role_on_user_project(self):
         )
         self.assertIsNone(result)
 
+    def test_role_add_with_error(self):
+        arglist = [
+            identity_fakes.role_name,
+        ]
+        verifylist = [
+            ('user', None),
+            ('group', None),
+            ('domain', None),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', False),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+
 
 class TestRoleAddInherited(TestRoleAdd, TestRoleInherited):
     pass
@@ -771,6 +787,17 @@ def test_role_list_domain_role(self):
         ), )
         self.assertEqual(datalist, tuple(data))
 
+    def test_role_list_group_with_error(self):
+        arglist = [
+            '--group', identity_fakes.group_id,
+        ]
+        verifylist = [
+            ('group', identity_fakes.group_id),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+
 
 class TestRoleRemove(TestRole):
 
@@ -982,6 +1009,22 @@ def test_role_remove_domain_role_on_group_domain(self):
         )
         self.assertIsNone(result)
 
+    def test_role_remove_with_error(self):
+        arglist = [
+            identity_fakes.role_name,
+        ]
+        verifylist = [
+            ('user', None),
+            ('group', None),
+            ('domain', None),
+            ('project', None),
+            ('role', identity_fakes.role_name),
+            ('inherited', False),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+
 
 class TestRoleSet(TestRole):