From cfd4e2a7228c1e7f6ad677f2dd6dbd09e638dfb7 Mon Sep 17 00:00:00 2001 From: Huanxuan Ao Date: Thu, 9 Feb 2017 18:05:32 +0800 Subject: [PATCH] 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 --- openstackclient/identity/v3/group.py | 26 ++++------- openstackclient/identity/v3/role.py | 29 +++++-------- .../functional/identity/v3/test_group.py | 25 ++--------- .../tests/unit/identity/v3/test_group.py | 29 +++++++++++++ .../tests/unit/identity/v3/test_role.py | 43 +++++++++++++++++++ 5 files changed, 94 insertions(+), 58 deletions(-) diff --git a/openstackclient/identity/v3/group.py b/openstackclient/identity/v3/group.py index a03a86ebd2..2afdabc171 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") % { + 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.stderr.write(msg) - else: - msg = _("%(user)s added to group %(group)s\n") % { - 'user': parsed_args.user, - 'group': parsed_args.group, - } - 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") % { + 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.stderr.write(msg) - else: - msg = _("%(user)s removed from group %(group)s\n") % { - 'user': parsed_args.user, - 'group': parsed_args.group, - } - 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 994ecc9c8b..1bbf5f07b0 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): ' --project --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 7049118359..917d5df048 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 8558de950a..00bd217dad 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 c0b68bdfad..39dbd24466 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):