Merge "Modify error handling for role and group commands"

5 files changed, 94 insertions, 58 deletions

diff --git a/openstackclient/identity/v3/group.py b/openstackclient/identity/v3/group.py
index a03a86eb..2afdabc1 100644
--- a/openstackclient/identity/v3/group.py
+++ b/openstackclient/identity/v3/group.py
@@ -62,18 +62,13 @@ class AddUserToGroup(command.Command):
 
         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 @@ class RemoveUserFromGroup(command.Command):
 
         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):
diff --git a/openstackclient/identity/v3/role.py b/openstackclient/identity/v3/role.py
index 994ecc9c..1bbf5f07 100644
--- a/openstackclient/identity/v3/role.py
+++ b/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 @@ class AddRole(command.Command):
 
         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 @@ class AddRole(command.Command):
 
         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 @@ class ListRole(command.Lister):
                                '<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 @@ class RemoveRole(command.Command):
 
         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 @@ class RemoveRole(command.Command):
 
         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)
 
 
diff --git a/openstackclient/tests/functional/identity/v3/test_group.py b/openstackclient/tests/functional/identity/v3/test_group.py
index 70491183..917d5df0 100644
--- a/openstackclient/tests/functional/identity/v3/test_group.py
+++ b/openstackclient/tests/functional/identity/v3/test_group.py
@@ -102,11 +102,7 @@ class GroupTests(common.IdentityTests):
                                     '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 @@ class GroupTests(common.IdentityTests):
                                     '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 @@ class GroupTests(common.IdentityTests):
                                     '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)
diff --git a/openstackclient/tests/unit/identity/v3/test_group.py b/openstackclient/tests/unit/identity/v3/test_group.py
index 8558de95..00bd217d 100644
--- a/openstackclient/tests/unit/identity/v3/test_group.py
+++ b/openstackclient/tests/unit/identity/v3/test_group.py
@@ -70,6 +70,20 @@ class TestGroupAddUser(TestGroup):
             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 @@ class TestGroupRemoveUser(TestGroup):
             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):
 
diff --git a/openstackclient/tests/unit/identity/v3/test_role.py b/openstackclient/tests/unit/identity/v3/test_role.py
index c0b68bdf..39dbd244 100644
--- a/openstackclient/tests/unit/identity/v3/test_role.py
+++ b/openstackclient/tests/unit/identity/v3/test_role.py
@@ -273,6 +273,22 @@ class TestRoleAdd(TestRole):
         )
         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 @@ class TestRoleList(TestRole):
         ), )
         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 @@ class TestRoleRemove(TestRole):
         )
         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):