Merge "Fix wrong order of positional args in cli"

2 files changed, 86 insertions, 7 deletions

diff --git a/oslo/config/cfg.py b/oslo/config/cfg.py
index 63aecff..a6fe1a1 100644
--- a/oslo/config/cfg.py
+++ b/oslo/config/cfg.py
@@ -1570,8 +1570,17 @@ class _CachedArgumentParser(argparse.ArgumentParser):
         self._args_cache[container] = values
 
     def initialize_parser_arguments(self):
+        # NOTE(mfedosin): The code below looks a little bit weird, but
+        # it's done because we need to sort only optional opts and do
+        # not touch positional. For the reason optional opts go first in
+        # the values we only need to find an index of the first positional
+        # option and then sort the values slice.
         for container, values in six.iteritems(self._args_cache):
-            values.sort(key=lambda x: x['args'])
+            index = 0
+            for index, argument in enumerate(values):
+                if not argument['args'][0].startswith('-'):
+                    break
+            values[:index] = sorted(values[:index], key=lambda x: x['args'])
             for argument in values:
                 try:
                     container.add_argument(*argument['args'],
@@ -1613,6 +1622,7 @@ class ConfigOpts(collections.Mapping):
         self._namespace = None
         self.__cache = {}
         self._config_opts = []
+        self._cli_opts = collections.deque()
         self._validate_default_values = False
 
     def _pre_setup(self, project, prog, version, usage, default_config_files):
@@ -1781,6 +1791,14 @@ class ConfigOpts(collections.Mapping):
         for group in self._groups.values():
             group._clear()
 
+    def _add_cli_opt(self, opt, group):
+        if {'opt': opt, 'group': group} in self._cli_opts:
+            return
+        if opt.positional:
+            self._cli_opts.append({'opt': opt, 'group': group})
+        else:
+            self._cli_opts.appendleft({'opt': opt, 'group': group})
+
     @__clear_cache
     def register_opt(self, opt, group=None, cli=False):
         """Register an option schema.
@@ -1797,8 +1815,13 @@ class ConfigOpts(collections.Mapping):
         """
         if group is not None:
             group = self._get_group(group, autocreate=True)
+            if cli:
+                self._add_cli_opt(opt, group)
             return group._register_opt(opt, cli)
 
+        if cli:
+            self._add_cli_opt(opt, None)
+
         if _is_opt_registered(self._opts, opt):
             return False
 
@@ -1860,6 +1883,9 @@ class ConfigOpts(collections.Mapping):
         if self._args is not None:
             raise ArgsAlreadyParsedError("reset before unregistering options")
 
+        if {'opt': opt, 'group': group} in self._cli_opts:
+            self._cli_opts.remove({'opt': opt, 'group': group})
+
         if group is not None:
             self._get_group(group)._unregister_opt(opt)
         elif opt.dest in self._opts:
@@ -1975,9 +2001,8 @@ class ConfigOpts(collections.Mapping):
 
     def _all_cli_opts(self):
         """A generator function for iterating CLI opts."""
-        for info, group in self._all_opt_infos():
-            if info['cli']:
-                yield info['opt'], group
+        for item in self._cli_opts:
+            yield item['opt'], item['group']
 
     def _unset_defaults_and_overrides(self):
         """Unset any default or override on all options."""
@@ -2267,9 +2292,7 @@ class ConfigOpts(collections.Mapping):
 
         """
         self._args = args
-
-        for opt, group in sorted(self._all_cli_opts(),
-                                 key=lambda x: x[0].name):
+        for opt, group in self._all_cli_opts():
             opt._add_to_cli(self._oparser, group)
 
         return self._parse_config_files()
diff --git a/tests/test_cfg.py b/tests/test_cfg.py
index 423f42d..c9c25c9 100644
--- a/tests/test_cfg.py
+++ b/tests/test_cfg.py
@@ -622,6 +622,62 @@ class PositionalTestCase(BaseTestCase):
             cfg.StrOpt('foo', required=True, positional=True))
         self.assertRaises(cfg.RequiredOptError, self.conf, [])
 
+    def test_positional_opts_order(self):
+        self.conf.register_cli_opts((
+            cfg.StrOpt('command', positional=True),
+            cfg.StrOpt('arg1', positional=True),
+            cfg.StrOpt('arg2', positional=True))
+        )
+
+        self.conf(['command', 'arg1', 'arg2'])
+
+        self.assertEqual('command', self.conf.command)
+        self.assertEqual('arg1', self.conf.arg1)
+        self.assertEqual('arg2', self.conf.arg2)
+
+    def test_positional_opt_order(self):
+        self.conf.register_cli_opt(
+            cfg.StrOpt('command', positional=True))
+        self.conf.register_cli_opt(
+            cfg.StrOpt('arg1', positional=True))
+        self.conf.register_cli_opt(
+            cfg.StrOpt('arg2', positional=True))
+
+        self.conf(['command', 'arg1', 'arg2'])
+
+        self.assertEqual('command', self.conf.command)
+        self.assertEqual('arg1', self.conf.arg1)
+        self.assertEqual('arg2', self.conf.arg2)
+
+    def test_positional_opt_unregister(self):
+        command = cfg.StrOpt('command', positional=True)
+        arg1 = cfg.StrOpt('arg1', positional=True)
+        arg2 = cfg.StrOpt('arg2', positional=True)
+        self.conf.register_cli_opt(command)
+        self.conf.register_cli_opt(arg1)
+        self.conf.register_cli_opt(arg2)
+
+        self.conf(['command', 'arg1', 'arg2'])
+
+        self.assertEqual('command', self.conf.command)
+        self.assertEqual('arg1', self.conf.arg1)
+        self.assertEqual('arg2', self.conf.arg2)
+
+        self.conf.reset()
+
+        self.conf.unregister_opt(arg1)
+        self.conf.unregister_opt(arg2)
+
+        arg0 = cfg.StrOpt('arg0', positional=True)
+        self.conf.register_cli_opt(arg0)
+        self.conf.register_cli_opt(arg1)
+
+        self.conf(['command', 'arg0', 'arg1'])
+
+        self.assertEqual('command', self.conf.command)
+        self.assertEqual('arg0', self.conf.arg0)
+        self.assertEqual('arg1', self.conf.arg1)
+
 
 class ConfigFileOptsTestCase(BaseTestCase):