Reduce redundant parameter of some commands in CLI

When deleting an alarm, we use "ceilometer alarm-delete -a <ALARM_ID>", 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 [<ALARM_ID>] Show an alarm. Positional arguments: <ALARM_ID> 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 <nsaje@redhat.com> Change-Id: I1fbc85aa253929bfbb5e73ed834a725b9cf828b4 Closes-bug: #1268557
2014-01-15 16:31:55 +08:00
parent 4cef2bf39a
commit 97e9c254e3
3 changed files with 79 additions and 32 deletions
--- 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
--- 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)
--- 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='<QUERY>',
           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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', nargs='?',
           action=NotEmptyAction, help='ID of the alarm state to set.')
@utils.arg('--state', metavar='<STATE>', 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='<ALARM_ID>', required=True,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', 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='<ALARM_ID>', required=True,
-           action=NotEmptyAction,
+@utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>',
+           action=obsoleted_by('alarm_id'), help=argparse.SUPPRESS,
+           dest='alarm_id_deprecated')
+@utils.arg('alarm_id', metavar='<ALARM_ID>', nargs='?', action=NotEmptyAction,
           help='ID of the alarm for which history is shown.')
@utils.arg('-q', '--query', metavar='<QUERY>',
           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='<RESOURCE_ID>', required=True,
+@utils.arg('resource_id', metavar='<RESOURCE_ID>',
           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='<message_id>',
-           help='The ID of the event. Should be a UUID.',
-           required=True, action=NotEmptyAction)
+@utils.arg('message_id', metavar='<message_id>', 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)