Improve cinder-manage arg parsing

Cinder manage command has some serious limitations to parse args such
as:

- Optional args cannot have non leading dashes (for non optional we can
  use metavar to display a different name).
- argparse `dest` option is ignored.
- All arguments are positional instead of keyword arguments.

This patch fixes these issues so now we can have optional arguments with
dashes (will be converted to underscores), arguments are passed as
keyword arguments instead of positional, and we honor `dest`
configuration option.

Change-Id: I51df05b52bf05c3e9227bc80b4435ed4a7a555f7

cinder/cmd/manage.py

@@ -117,7 +117,7 @@ class ShellCommands(object):
         """
         self.run('python')
 
-    @args('--shell', dest="shell",
+    @args('--shell',
           metavar='<bpython|ipython|python>',
           help='Python shell')
     def run(self, shell=None):
@@ -539,29 +539,32 @@ category_opt = cfg.SubCommandOpt('category',
 
 
 def get_arg_string(args):
-    arg = None
     if args[0] == '-':
         # (Note)zhiteng: args starts with FLAGS.oparser.prefix_chars
         # is optional args. Notice that cfg module takes care of
         # actual ArgParser so prefix_chars is always '-'.
         if args[1] == '-':
             # This is long optional arg
-            arg = args[2:]
+            args = args[2:]
         else:
-            arg = args[1:]
-    else:
-        arg = args
+            args = args[1:]
 
-    return arg
+    # We convert dashes to underscores so we can have cleaner optional arg
+    # names
+    if args:
+        args = args.replace('-', '_')
+
+    return args
 
 
 def fetch_func_args(func):
-    fn_args = []
+    fn_kwargs = {}
     for args, kwargs in getattr(func, 'args', []):
-        arg = get_arg_string(args[0])
-        fn_args.append(getattr(CONF.category, arg))
+        # Argparser `dest` configuration option takes precedence for the name
+        arg = kwargs.get('dest') or get_arg_string(args[0])
+        fn_kwargs[arg] = getattr(CONF.category, arg)
 
-    return fn_args
+    return fn_kwargs
 
 
 def main():
@@ -600,5 +603,5 @@ def main():
         sys.exit(2)
 
     fn = CONF.category.action_fn
-    fn_args = fetch_func_args(fn)
-    fn(*fn_args)
+    fn_kwargs = fetch_func_args(fn)
+    fn(**fn_kwargs)

cinder/tests/unit/test_cmd.py

@@ -13,6 +13,7 @@
 import datetime
 import sys
 
+import ddt
 import mock
 from oslo_config import cfg
 import six
@@ -380,6 +381,7 @@ class TestCinderVolumeCmd(test.TestCase):
         launcher.wait.assert_called_once_with()
 
 
+@ddt.ddt
 class TestCinderManageCmd(test.TestCase):
 
     def setUp(self):
@@ -776,14 +778,28 @@ class TestCinderManageCmd(test.TestCase):
             service['binary'] = 'cinder-%s' % binary
             self._test_service_commands_list(service)
 
-    def test_get_arg_string(self):
-        args1 = "foobar"
-        args2 = "-foo bar"
-        args3 = "--foo bar"
+    @ddt.data(('foobar', 'foobar'), ('-foo bar', 'foo bar'),
+              ('--foo bar', 'foo bar'), ('--foo-bar', 'foo_bar'),
+              ('---foo-bar', '_foo_bar'))
+    @ddt.unpack
+    def test_get_arg_string(self, arg, expected):
+        self.assertEqual(expected, cinder_manage.get_arg_string(arg))
 
-        self.assertEqual("foobar", cinder_manage.get_arg_string(args1))
-        self.assertEqual("foo bar", cinder_manage.get_arg_string(args2))
-        self.assertEqual("foo bar", cinder_manage.get_arg_string(args3))
+    def test_fetch_func_args(self):
+        @cinder_manage.args('--full-rename')
+        @cinder_manage.args('--different-dest', dest='my_dest')
+        @cinder_manage.args('current')
+        def my_func():
+            pass
+
+        expected = {'full_rename': mock.sentinel.full_rename,
+                    'my_dest': mock.sentinel.my_dest,
+                    'current': mock.sentinel.current}
+
+        with mock.patch.object(cinder_manage, 'CONF') as mock_conf:
+            mock_conf.category = mock.Mock(**expected)
+            self.assertDictEqual(expected,
+                                 cinder_manage.fetch_func_args(my_func))
 
     @mock.patch('oslo_config.cfg.ConfigOpts.register_cli_opt')
     def test_main_argv_lt_2(self, register_cli_opt):