Refactor set_argument_parser to fix shell regression

Openstack Client and Validation Shell is hitting a regression
due to set_argument_parser calling directly parse_known_args
from argparse.
This patch fix the regression and avoid the need to call
parse_known_args directly.

Closes-Bug: #1949596
Change-Id: Ic9a682e7c3d5431e8779064947bd379c00aba584

validations_libs/cli/base.py

@@ -15,6 +15,7 @@
 #   under the License.
 
 import os
+import sys
 
 from cliff import _argparse
 from cliff.command import Command
@@ -35,38 +36,29 @@ class Base:
     """Base class for CLI arguments management"""
     config = {}
 
-    def _format_arg(self, parser):
+    def set_argument_parser(self, vf_parser, args, section='default'):
-        """Format arguments parser"""
-        namespace, argv = parser.parse_known_args()
-        return [arg.lstrip(parser.prefix_chars).replace('-', '_')
-                for arg in argv]
-
-    def set_argument_parser(self, parser, section='default'):
         """ Set Arguments parser depending of the precedence ordering:
             * User CLI arguments
             * Configuration file
             * Default CLI values
         """
-        cli_args = self._format_arg(parser)
+        # load parser
-        args, _ = parser.parse_known_args()
+        parser = vf_parser.get_parser(vf_parser)
+        # load cli args and skip binary and action
+        cli_args = sys.argv[2:]
+        cli_key = [arg.lstrip(parser.prefix_chars).replace('-', '_')
+                   for arg in cli_args if arg.startswith('--')]
+
         self.config = utils.load_config(os.path.abspath(args.config))
         config_args = self.config.get(section, {})
         for key, value in args._get_kwargs():
-            # matbu: manage the race when user's cli arg is the same than
+            if key in cli_key:
-            # the parser default value. The user's cli arg will *always*
+                config_args.update({key: value})
-            # takes precedence on others.
-            if parser.get_default(key) == value and key in cli_args:
-                try:
-                    cli_value = cli_args[cli_args.index(key)+1]
-                    config_args.update({key: cli_value})
-                except KeyError:
-                    print('Key not found in cli: {}').format(key)
             elif parser.get_default(key) != value:
                 config_args.update({key: value})
             elif key not in config_args.keys():
                 config_args.update({key: value})
-        parser.set_defaults(**config_args)
+        return vars(args).update(**config_args)
-        return parser
 
 
 class BaseCommand(Command):

validations_libs/cli/community.py

@@ -48,13 +48,12 @@ class CommunityValidationInit(BaseCommand):
             )
         )
 
-        if self.app:
-            # Merge config and CLI args:
-            return self.base.set_argument_parser(parser)
         return parser
 
     def take_action(self, parsed_args):
         """Take Community Validation Action"""
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
 
         co_validation = com_val(parsed_args.validation_name)
 

validations_libs/cli/history.py

@@ -45,10 +45,12 @@ class ListHistory(BaseLister):
                             default=constants.VALIDATIONS_LOG_BASEDIR,
                             help=("Path where the validation log files "
                                   "is located."))
-        # Merge config and CLI args:
+        return parser
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
+
         validation_log_dir = parsed_args.validation_log_dir
         history_limit = parsed_args.history_limit
 
@@ -86,10 +88,12 @@ class GetHistory(BaseCommand):
                             default=constants.VALIDATIONS_LOG_BASEDIR,
                             help=("Path where the validation log files "
                                   "is located."))
-        # Merge config and CLI args:
+        return parser
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
+
         self.app.LOG.debug(
             (
                 "Obtaining information about the validation run {}\n"

validations_libs/cli/lister.py

@@ -51,11 +51,12 @@ class ValidationList(BaseLister):
                             default=constants.ANSIBLE_VALIDATION_DIR,
                             help=("Path where the validation playbooks "
                                   "are located."))
-        # Merge config and CLI args:
+        return parser
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
         """Take validation action"""
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
 
         group = parsed_args.group
         category = parsed_args.category

validations_libs/cli/run.py

@@ -162,14 +162,12 @@ class Run(BaseCommand):
             help=("Run specific validations by product, "
                   "if more than one product is required "
                   "separate the product names with commas."))
-
-        if self.app:
-            # Merge config and CLI args:
-            return self.base.set_argument_parser(parser)
         return parser
 
     def take_action(self, parsed_args):
         """Take validation action"""
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
         # Get config:
         config = self.base.config
 

validations_libs/cli/show.py

@@ -34,11 +34,12 @@ class Show(BaseShow):
                             metavar="<validation>",
                             type=str,
                             help="Show a specific validation.")
-        # Merge config and CLI args:
+        return parser
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
         """Take validation action"""
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
         # Get parameters:
         validation_dir = parsed_args.validation_dir
         validation_name = parsed_args.validation_name
@@ -61,12 +62,12 @@ class ShowGroup(BaseLister):
                             default=constants.ANSIBLE_VALIDATION_DIR,
                             help=("Path where the validation playbooks "
                                   "are located."))
-
+        return parser
-        # Merge config and CLI args:
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
         """Take validation action"""
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
 
         v_actions = ValidationActions(parsed_args.validation_dir)
         return v_actions.group_information(constants.VALIDATION_GROUPS_INFO)
@@ -147,10 +148,11 @@ class ShowParameter(BaseShow):
             help=("Print representation of the validation. "
                   "The choices of the output format is json,yaml. ")
         )
-        # Merge config and CLI args:
+        return parser
-        return self.base.set_argument_parser(parser)
 
     def take_action(self, parsed_args):
+        # Merge config and CLI args:
+        self.base.set_argument_parser(self, parsed_args)
 
         validation_dir = parsed_args.validation_dir
         v_actions = ValidationActions(validation_dir)

validations_libs/tests/cli/test_app.py

@@ -45,7 +45,6 @@ class TestArgApp(TestCase):
         parsed_args = parser.parse_args(args)
         self.assertEqual('foo', parsed_args.validation_dir)
 
-    @mock.patch('validations_libs.constants.ANSIBLE_VALIDATION_DIR', 'bar')
     @mock.patch('validations_libs.utils.find_config_file',
                 return_value='validation.cfg')
     def test_validation_dir_config_no_cli(self, mock_config):

validations_libs/tests/cli/test_base.py

@@ -42,16 +42,6 @@ class TestBase(BaseCommand):
         self.cmd = lister.ValidationList(self.app, None)
         self.base = base.Base()
 
-    @mock.patch('argparse.ArgumentParser.parse_known_args',
-                return_value=(TestArgParse(), ['foo-bar']))
-    @mock.patch('os.path.abspath', return_value='/foo')
-    @mock.patch('validations_libs.utils.load_config',
-                return_value=fakes.DEFAULT_CONFIG)
-    def test_config_args(self, mock_config, mock_path, mock_argv):
-        cmd_parser = self.cmd.get_parser('check_parser')
-
-        self.assertEqual(['foo_bar'], self.base._format_arg(cmd_parser))
-
     @mock.patch('os.path.abspath', return_value='/foo')
     @mock.patch('validations_libs.utils.load_config',
                 return_value=fakes.DEFAULT_CONFIG)
@@ -59,36 +49,36 @@ class TestBase(BaseCommand):
         arglist = ['--validation-dir', 'foo', '--config', 'validation.cfg']
         verifylist = [('validation_dir', 'foo')]
         self._set_args(arglist)
-        cmd_parser = self.cmd.get_parser('check_parser')
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        parser = self.base.set_argument_parser(cmd_parser)
+        self.base.set_argument_parser(self.cmd, parsed_args)
 
         self.assertEqual(fakes.DEFAULT_CONFIG, self.base.config)
-        self.assertEqual(parser.get_default('validation_dir'), 'foo')
+        self.assertEqual(parsed_args.validation_dir, 'foo')
 
     @mock.patch('os.path.abspath', return_value='/foo')
     @mock.patch('validations_libs.utils.load_config',
                 return_value=fakes.DEFAULT_CONFIG)
     def test_argument_parser_config_choice(self, mock_load, mock_path):
-        arglist = []
+        arglist = ['--config', 'validation.cfg']
         verifylist = []
         self._set_args(arglist)
-        cmd_parser = self.cmd.get_parser('check_parser')
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        parser = self.base.set_argument_parser(cmd_parser)
+        self.base.set_argument_parser(self.cmd, parsed_args)
 
         self.assertEqual(fakes.DEFAULT_CONFIG, self.base.config)
-        self.assertEqual(parser.get_default('validation_dir'),
+        self.assertEqual(parsed_args.validation_dir,
                          '/usr/share/ansible/validation-playbooks')
 
     @mock.patch('os.path.abspath', return_value='/foo')
     @mock.patch('validations_libs.utils.load_config',
                 return_value={})
     def test_argument_parser_constant_choice(self, mock_load, mock_path):
-        arglist = []
+        arglist = ['--config', 'validation.cfg']
         verifylist = []
         self._set_args(arglist)
-        cmd_parser = self.cmd.get_parser('check_parser')
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        parser = self.base.set_argument_parser(cmd_parser)
+        self.base.set_argument_parser(self.cmd, parsed_args)
 
         self.assertEqual({}, self.base.config)
-        self.assertEqual(parser.get_default('validation_dir'),
+        self.assertEqual(parsed_args.validation_dir,
                          '/usr/share/ansible/validation-playbooks')

validations_libs/tests/cli/test_history.py

@@ -67,10 +67,9 @@ class TestListHistory(BaseCommand):
         verifylist = [('validation_log_dir', '/foo/log/dir')]
         self._set_args(arglist)
 
-        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        self.assertEqual(parsed_args.history_limit, 0)
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
         self.assertRaises(ValueError, self.cmd.take_action, parsed_args)
+        self.assertEqual(parsed_args.history_limit, 0)
 
 
 class TestGetHistory(BaseCommand):