Merge "Improve cinder-manage arg parsing"

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):