From 97e9c254e3e7113bb096be689fa9159293b3920b Mon Sep 17 00:00:00 2001 From: ls1175 Date: Wed, 15 Jan 2014 16:31:55 +0800 Subject: Reduce redundant parameter of some commands in CLI When deleting an alarm, we use "ceilometer alarm-delete -a ", unlike other deleting commands of openstack, the parameter-a/--alarm_id is redundant. The similar situations exist in showing alarm, geting alarm state, showing resource and so on. It is more easy to use for reducing these parameters. New behaviour: $ ceilometer help alarm-show usage: ceilometer alarm-show [] Show an alarm. Positional arguments: ID of the alarm to show. $ ceilometer alarm-show alarm_id should not be empty $ ceilometer alarm-show abcde Not Found (HTTP 404) $ ceilometer alarm-show -a abcde -a is obsolete! See help for more details. Not Found (HTTP 404) $ ceilometer alarm-show --alarm_id abcde --alarm_id is obsolete! See help for more details. Not Found (HTTP 404) Co-Authored-By: Nejc Saje Change-Id: I1fbc85aa253929bfbb5e73ed834a725b9cf828b4 Closes-bug: #1268557 --- ceilometerclient/shell.py | 15 +++------ ceilometerclient/tests/v2/test_shell.py | 38 ++++++++++++++++----- ceilometerclient/v2/shell.py | 58 +++++++++++++++++++++++++-------- 3 files changed, 79 insertions(+), 32 deletions(-) diff --git a/ceilometerclient/shell.py b/ceilometerclient/shell.py index 138dd62..d16dc51 100644 --- a/ceilometerclient/shell.py +++ b/ceilometerclient/shell.py @@ -256,17 +256,10 @@ class CeilometerShell(object): class HelpFormatter(argparse.HelpFormatter): - INDENT_BEFORE_ARGUMENTS = 6 - MAX_WIDTH_ARGUMENTS = 32 - - def add_arguments(self, actions): - for action in filter(lambda x: not x.option_strings, actions): - for choice in action.choices: - length = len(choice) + self.INDENT_BEFORE_ARGUMENTS - if(length > self._max_help_position and - length <= self.MAX_WIDTH_ARGUMENTS): - self._max_help_position = length - super(HelpFormatter, self).add_arguments(actions) + def __init__(self, prog, indent_increment=2, max_help_position=32, + width=None): + super(HelpFormatter, self).__init__(prog, indent_increment, + max_help_position, width) def start_section(self, heading): # Title-case the headings diff --git a/ceilometerclient/tests/v2/test_shell.py b/ceilometerclient/tests/v2/test_shell.py index 7da088a..632684f 100644 --- a/ceilometerclient/tests/v2/test_shell.py +++ b/ceilometerclient/tests/v2/test_shell.py @@ -260,8 +260,7 @@ class ShellAlarmCommandTest(utils.BaseTestCase): self._test_alarm_threshold_action_args('create', argv) def test_alarm_threshold_update_args(self): - argv = ['alarm-threshold-update', - '--alarm_id', 'x'] + self.THRESHOLD_ALARM_CLI_ARGS + argv = ['alarm-threshold-update', 'x'] + self.THRESHOLD_ALARM_CLI_ARGS self._test_alarm_threshold_action_args('update', argv) @mock.patch('sys.stdout', new=six.StringIO()) @@ -804,14 +803,18 @@ class ShellStatisticsTest(utils.BaseTestCase): class ShellEmptyIdTest(utils.BaseTestCase): """Test empty field which will cause calling incorrect rest uri.""" - def _test_entity_action_with_empty_values(self, entity, *args): + def _test_entity_action_with_empty_values(self, entity, + *args, **kwargs): + positional = kwargs.pop('positional', False) for value in ('', ' ', ' ', '\t'): - self._test_entity_action_with_empty_value(entity, value, *args) + self._test_entity_action_with_empty_value(entity, value, + positional, *args) - def _test_entity_action_with_empty_value(self, entity, value, *args): + def _test_entity_action_with_empty_value(self, entity, value, + positional, *args): + new_args = [value] if positional else ['--%s' % entity, value] + argv = list(args) + new_args shell = base_shell.CeilometerShell() - argv = list(args) + ['--%s' % entity, value] - with mock.patch('ceilometerclient.exc.CommandError') as e: e.return_value = exc.BaseException() self.assertRaises(exc.BaseException, shell.parse_args, argv) @@ -820,7 +823,8 @@ class ShellEmptyIdTest(utils.BaseTestCase): def _test_alarm_action_with_empty_ids(self, method, *args): args = [method] + list(args) - self._test_entity_action_with_empty_values('alarm_id', *args) + self._test_entity_action_with_empty_values('alarm_id', + positional=True, *args) def test_alarm_show_with_empty_id(self): self._test_alarm_action_with_empty_ids('alarm-show') @@ -879,3 +883,21 @@ class ShellEmptyIdTest(utils.BaseTestCase): def test_trait_list_with_empty_trait_name(self): args = ['trait-list', '--event_type', 'x'] self._test_entity_action_with_empty_values('trait_name', *args) + + +class ShellObsoletedArgsTest(utils.BaseTestCase): + """Test arguments that have been obsoleted.""" + + def _test_entity_obsoleted(self, entity, value, positional, *args): + new_args = [value] if positional else ['--%s' % entity, value] + argv = list(args) + new_args + shell = base_shell.CeilometerShell() + with mock.patch('sys.stdout', new_callable=six.StringIO) as stdout: + shell.parse_args(argv) + self.assertIn('obsolete', stdout.getvalue()) + + def test_obsolete_alarm_id(self): + for method in ['alarm-show', 'alarm-update', 'alarm-threshold-update', + 'alarm-combination-update', 'alarm-delete', + 'alarm-state-get', 'alarm-history']: + self._test_entity_obsoleted('alarm_id', 'abcde', False, method) diff --git a/ceilometerclient/v2/shell.py b/ceilometerclient/v2/shell.py index 72f780d..ce586e8 100644 --- a/ceilometerclient/v2/shell.py +++ b/ceilometerclient/v2/shell.py @@ -51,11 +51,21 @@ SIMPLE_OPERATORS = ["=", "!=", "<", "<=", '>', '>='] class NotEmptyAction(argparse.Action): def __call__(self, parser, namespace, values, option_string=None): + values = values or getattr(namespace, self.dest) if not values or values.isspace(): raise exc.CommandError('%s should not be empty' % self.dest) setattr(namespace, self.dest, values) +def obsoleted_by(new_dest): + class ObsoletedByAction(argparse.Action): + def __call__(self, parser, namespace, values, option_string=None): + old_dest = option_string or self.dest + print('%s is obsolete! See help for more details.' % old_dest) + setattr(namespace, new_dest, values) + return ObsoletedByAction + + @utils.arg('-q', '--query', metavar='', help='key[op]data_type::value; list. data_type is optional, ' 'but if supplied must be string, integer, float, or boolean.') @@ -333,7 +343,10 @@ def _display_alarm(alarm): utils.print_dict(data, wrap=72) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm to show.') def do_alarm_show(cc, args={}): '''Show an alarm.''' @@ -490,7 +503,10 @@ def do_alarm_combination_create(cc, args={}): _display_alarm(alarm) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm to update.') @common_alarm_arguments() @utils.arg('--remove-time-constraint', action='append', @@ -531,7 +547,10 @@ def do_alarm_update(cc, args={}): _display_alarm(alarm) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm to update.') @common_alarm_arguments() @utils.arg('--remove-time-constraint', action='append', @@ -583,7 +602,10 @@ def do_alarm_threshold_update(cc, args={}): _display_alarm(alarm) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm to update.') @common_alarm_arguments() @utils.arg('--remove-time-constraint', action='append', @@ -615,7 +637,10 @@ def do_alarm_combination_update(cc, args={}): _display_alarm(alarm) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm to delete.') def do_alarm_delete(cc, args={}): '''Delete an alarm.''' @@ -625,7 +650,10 @@ def do_alarm_delete(cc, args={}): raise exc.CommandError('Alarm not found: %s' % args.alarm_id) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm state to set.') @utils.arg('--state', metavar='', required=True, help='State of the alarm, one of: ' + str(ALARM_STATES) + @@ -639,7 +667,10 @@ def do_alarm_state_set(cc, args={}): utils.print_dict({'state': state}, wrap=72) -@utils.arg('-a', '--alarm_id', metavar='', required=True, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm state to show.') def do_alarm_state_get(cc, args={}): '''Get the state of an alarm.''' @@ -650,8 +681,10 @@ def do_alarm_state_get(cc, args={}): utils.print_dict({'state': state}, wrap=72) -@utils.arg('-a', '--alarm_id', metavar='', required=True, - action=NotEmptyAction, +@utils.arg('-a', '--alarm_id', metavar='', + action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS, + dest='alarm_id_deprecated') +@utils.arg('alarm_id', metavar='', nargs='?', action=NotEmptyAction, help='ID of the alarm for which history is shown.') @utils.arg('-q', '--query', metavar='', help='key[op]data_type::value; list. data_type is optional, ' @@ -688,7 +721,7 @@ def do_resource_list(cc, args={}): sortby=1) -@utils.arg('-r', '--resource_id', metavar='', required=True, +@utils.arg('resource_id', metavar='', action=NotEmptyAction, help='ID of the resource to show.') def do_resource_show(cc, args={}): '''Show the resource.''' @@ -719,9 +752,8 @@ def do_event_list(cc, args={}): )}) -@utils.arg('-m', '--message_id', metavar='', - help='The ID of the event. Should be a UUID.', - required=True, action=NotEmptyAction) +@utils.arg('message_id', metavar='', action=NotEmptyAction, + help='The ID of the event. Should be a UUID.') def do_event_show(cc, args={}): '''Show a particular event.''' event = cc.events.get(args.message_id) -- cgit v1.2.1