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

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='<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)