From ea5c3e7b28d5963ae9b64733f8e778ecd219b9a7 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 13 May 2016 16:54:20 +0300 Subject: [PATCH] Fix "single_alias" action for CLI commands For the moment if default value for CLI option with "single_alias" action is different than None we cannot redefine it. Fix it and cover with unit tests. Also, fix existing aliases and add some where good to have. Change-Id: Ic61426d9bd894b01f9b035d6d19bb7bd113f96ad Closes-Bug: #1552771 --- manilaclient/shell.py | 15 ++- manilaclient/tests/unit/test_shell.py | 146 ++++++++++++++++++++++++++ manilaclient/v2/shell.py | 13 ++- 3 files changed, 171 insertions(+), 3 deletions(-) diff --git a/manilaclient/shell.py b/manilaclient/shell.py index d7ce4db7b..8c2a2533a 100644 --- a/manilaclient/shell.py +++ b/manilaclient/shell.py @@ -55,10 +55,21 @@ class AllowOnlyOneAliasAtATimeAction(argparse.Action): def __call__(self, parser, namespace, values, option_string=None): # NOTE(vponomaryov): this method is redefinition of # argparse.Action.__call__ interface - if getattr(namespace, self.dest) is not None: + + if not hasattr(self, 'calls'): + self.calls = {} + + if self.dest not in self.calls: + self.calls[self.dest] = set() + + local_values = sorted(values) if isinstance(values, list) else values + self.calls[self.dest].add(six.text_type(local_values)) + + if len(self.calls[self.dest]) == 1: + setattr(namespace, self.dest, local_values) + else: msg = "Only one alias is allowed at a time." raise argparse.ArgumentError(self, msg) - setattr(namespace, self.dest, values) class ManilaClientArgumentParser(argparse.ArgumentParser): diff --git a/manilaclient/tests/unit/test_shell.py b/manilaclient/tests/unit/test_shell.py index 3137c813a..cee3e4e21 100644 --- a/manilaclient/tests/unit/test_shell.py +++ b/manilaclient/tests/unit/test_shell.py @@ -17,13 +17,16 @@ import ddt import fixtures import mock from six import moves +from tempest.lib.cli import output_parser from testtools import matchers import manilaclient from manilaclient.common import constants from manilaclient import exceptions +from manilaclient.openstack.common import cliutils from manilaclient import shell from manilaclient.tests.unit import utils +from manilaclient.tests.unit.v2 import fakes @ddt.ddt @@ -158,3 +161,146 @@ class OpenstackManilaShellTest(utils.TestCase): for expected_arg in expected_args: self.assertIn(expected_arg, help_text) + + +class CustomOpenStackManilaShell(shell.OpenStackManilaShell): + + @staticmethod + @cliutils.arg( + '--default-is-none', + '--default_is_none', + type=str, + metavar='', + action='single_alias', + help='Default value is None and metavar set.', + default=None) + def do_foo(cs, args): + cliutils.print_dict({'key': args.default_is_none}) + + @staticmethod + @cliutils.arg( + '--default-is-not-none', + '--default_is_not_none', + type=str, + action='single_alias', + help='Default value is not None and metavar not set.', + default='bar') + def do_bar(cs, args): + cliutils.print_dict({'key': args.default_is_not_none}) + + @staticmethod + @cliutils.arg( + '--list-like', + '--list_like', + nargs='*', + action='single_alias', + help='Default value is None, metavar not set and result is list.', + default=None) + def do_quuz(cs, args): + cliutils.print_dict({'key': args.list_like}) + + +@ddt.ddt +class AllowOnlyOneAliasAtATimeActionTest(utils.TestCase): + FAKE_ENV = { + 'OS_USERNAME': 'username', + 'OS_PASSWORD': 'password', + 'OS_TENANT_NAME': 'tenant_name', + 'OS_AUTH_URL': 'http://no.where', + } + + def setUp(self): + super(self.__class__, self).setUp() + for k, v in self.FAKE_ENV.items(): + self.useFixture(fixtures.EnvironmentVariable(k, v)) + self.mock_object( + shell.client, 'get_client_class', + mock.Mock(return_value=fakes.FakeClient)) + + def shell_discover_client(self, + current_client, + os_api_version, + os_endpoint_type, + os_service_type, + client_args): + return current_client, manilaclient.API_MAX_VERSION + + def shell(self, argstr): + orig = sys.stdout + try: + sys.stdout = moves.StringIO() + _shell = CustomOpenStackManilaShell() + _shell._discover_client = self.shell_discover_client + _shell.main(argstr.split()) + except SystemExit: + exc_type, exc_value, exc_traceback = sys.exc_info() + self.assertEqual(exc_value.code, 0) + finally: + out = sys.stdout.getvalue() + sys.stdout.close() + sys.stdout = orig + + return out + + @ddt.data( + ('--default-is-none foo', 'foo'), + ('--default-is-none foo --default-is-none foo', 'foo'), + ('--default-is-none foo --default_is_none foo', 'foo'), + ('--default_is_none None', 'None'), + ) + @ddt.unpack + def test_foo_success(self, options_str, expected_result): + output = self.shell('foo %s' % options_str) + parsed_output = output_parser.details(output) + self.assertEqual({'key': expected_result}, parsed_output) + + @ddt.data( + '--default-is-none foo --default-is-none bar', + '--default-is-none foo --default_is_none bar', + '--default-is-none foo --default_is_none FOO', + ) + def test_foo_error(self, options_str): + self.assertRaises( + matchers.MismatchError, self.shell, 'foo %s' % options_str) + + @ddt.data( + ('--default-is-not-none bar', 'bar'), + ('--default_is_not_none bar --default-is-not-none bar', 'bar'), + ('--default_is_not_none bar --default_is_not_none bar', 'bar'), + ('--default-is-not-none not_bar', 'not_bar'), + ('--default_is_not_none None', 'None'), + ) + @ddt.unpack + def test_bar_success(self, options_str, expected_result): + output = self.shell('bar %s' % options_str) + parsed_output = output_parser.details(output) + self.assertEqual({'key': expected_result}, parsed_output) + + @ddt.data( + '--default-is-not-none foo --default-is-not-none bar', + '--default-is-not-none foo --default_is_not_none bar', + '--default-is-not-none bar --default_is_not_none BAR', + ) + def test_bar_error(self, options_str): + self.assertRaises( + matchers.MismatchError, self.shell, 'bar %s' % options_str) + + @ddt.data( + ('--list-like q=w', "['q=w']"), + ('--list-like q=w --list_like q=w', "['q=w']"), + ('--list-like q=w e=r t=y --list_like e=r t=y q=w', + "['e=r', 'q=w', 't=y']"), + ('--list_like q=w e=r t=y', "['e=r', 'q=w', 't=y']"), + ) + @ddt.unpack + def test_quuz_success(self, options_str, expected_result): + output = self.shell('quuz %s' % options_str) + parsed_output = output_parser.details(output) + self.assertEqual({'key': expected_result}, parsed_output) + + @ddt.data( + '--list-like q=w --list_like e=r t=y', + ) + def test_quuz_error(self, options_str): + self.assertRaises( + matchers.MismatchError, self.shell, 'quuz %s' % options_str) diff --git a/manilaclient/v2/shell.py b/manilaclient/v2/shell.py index 4e305b531..02a039478 100644 --- a/manilaclient/v2/shell.py +++ b/manilaclient/v2/shell.py @@ -401,9 +401,11 @@ def do_quota_defaults(cs, args): help='New value for the "snapshot_gigabytes" quota.') @cliutils.arg( '--share-networks', + '--share_networks', metavar='', type=int, default=None, + action='single_alias', help='New value for the "share_networks" quota.') @cliutils.arg( '--force', @@ -529,7 +531,9 @@ def do_rate_limits(cs, args): help='Share size in GiB.') @cliutils.arg( '--snapshot-id', + '--snapshot_id', metavar='', + action='single_alias', help='Optional snapshot ID to create the share from. (Default=None)', default=None) @cliutils.arg( @@ -546,7 +550,9 @@ def do_rate_limits(cs, args): default=None) @cliutils.arg( '--share-network', + '--share_network', metavar='', + action='single_alias', help='Optional network info ID or name.', default=None) @cliutils.arg( @@ -624,6 +630,7 @@ def do_create(cs, args): metavar='', choices=['True', 'False'], required=False, + action='single_alias', help='Enables or disables generic host-based force-migration, which ' 'bypasses driver optimizations. Default=False.', default=False) @@ -705,6 +712,7 @@ def do_migration_cancel(cs, args): '--state', metavar='', default='migration_error', + action='single_alias', help=('Indicate which task state to assign the share. Options include ' 'migration_starting, migration_in_progress, migration_completing, ' 'migration_success, migration_error, migration_cancelled, ' @@ -1082,6 +1090,7 @@ def do_show(cs, args): type=str, default=None, choices=['rw', 'ro'], + action='single_alias', help='Share access level ("rw" and "ro" access levels are supported). ' 'Defaults to rw.') def do_access_allow(cs, args): @@ -1177,7 +1186,7 @@ def do_access_list(cs, args): 'was used for share creation. OPTIONAL: Default=None', default=None) @cliutils.arg( - '--share-type', '--volume-type' + '--share-type', '--volume-type', '--share_type', '--share-type-id', '--volume-type-id', # aliases '--share-type_id', '--share_type-id', '--share_type_id', # aliases '--volume_type', '--volume_type_id', @@ -3311,6 +3320,7 @@ def do_share_replica_list(cs, args): '--share_network', metavar='', default=None, + action='single_alias', help='Optional network info ID or name.') @api_versions.wraps("2.11") @api_versions.experimental_api @@ -3418,6 +3428,7 @@ def do_share_replica_reset_state(cs, args): '--state', # alias for user sanity metavar='', default='out_of_sync', + action='single_alias', help=('Indicate which replica_state to assign the replica. Options ' 'include in_sync, out_of_sync, active, error. If no ' 'state is provided, out_of_sync will be used.'))