From 607b1fbb2dcce2a139b4cb64c61e6072b9a0b880 Mon Sep 17 00:00:00 2001
From: mprahl <mprahl@redhat.com>
Date: Nov 09 2018 13:26:04 +0000
Subject: Validate the module build optional argument when parsing the argument


Signed-off-by: mprahl <mprahl@redhat.com>

---

diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py
index 6877e80..4d199d8 100644
--- a/pyrpkg/__init__.py
+++ b/pyrpkg/__init__.py
@@ -3492,8 +3492,8 @@ class Commands(object):
             'name:stream' to override.
         :param list requires: a list of requires in the format of 'name:stream'
             to override.
-        :param optional: an optional list of "key=value" to be passed in with
-            the MBS build submission.
+        :param list optional: an optional list of tuples (key, value) to be
+            passed in with the MBS build submission.
         :type optional: list[str]
         :param str oidc_id_provider: a string of the OIDC provider when MBS is
             using OIDC for authentication.
@@ -3510,13 +3510,9 @@ class Commands(object):
         body = {'scmurl': scm_url, 'branch': branch}
         optional = optional if optional else []
         optional_dict = {}
-        try:
-            for x in optional:
-                key, value = x.split('=', 1)
-                optional_dict[key] = value
-        except (IndexError, ValueError):
-            raise rpkgError(
-                'Optional arguments are not in the proper "key=value" format')
+        for option in optional:
+            key, value = option
+            optional_dict[key] = value
 
         for dep_type, overrides in (('buildrequires', buildrequires),
                                     ('requires', requires)):
@@ -3529,11 +3525,6 @@ class Commands(object):
                 body[key].setdefault(dep_name, [])
                 body[key][dep_name].append(dep_stream)
 
-        conflicting_keys = set(body.keys()) & set(optional_dict.keys())
-        if conflicting_keys:
-            raise rpkgError('The following optional arguments conflict with other arguments: {0}'
-                            .format(', '.join(conflicting_keys)))
-
         body.update(optional_dict)
         url = self.module_get_url(build_id=None, action='POST')
         resp = self.module_send_authorized_request(
diff --git a/pyrpkg/cli.py b/pyrpkg/cli.py
index e66ea4c..08755f7 100644
--- a/pyrpkg/cli.py
+++ b/pyrpkg/cli.py
@@ -1090,8 +1090,8 @@ defined, packages will be built sequentially.""" % {'name': self.name})
             dest='requires', type=utils.validate_module_dep_override,
             help='Requires to override in the form of "name:stream"')
         self.module_build_parser.add_argument(
-            '--optional', action='append', metavar='KEY=VALUE',
-            dest='optional',
+            '--optional', action='append', metavar='key=value',
+            dest='optional', type=utils.validate_module_build_optional,
             help='MBS optional arguments in the form of "key=value"')
         self.module_build_parser.set_defaults(command=self.module_build)
 
diff --git a/pyrpkg/utils.py b/pyrpkg/utils.py
index aa96254..b637911 100644
--- a/pyrpkg/utils.py
+++ b/pyrpkg/utils.py
@@ -113,3 +113,17 @@ def validate_module_dep_override(dep):
         return dep.split(':', 1)
     except (ValueError, AttributeError):
         raise argparse.ArgumentTypeError('This option must be in the format of "name:stream"')
+
+
+def validate_module_build_optional(optional_arg):
+    """Validate the passed-in optional argument to the module-build command."""
+    try:
+        key, value = optional_arg.split('=', 1)
+    except (ValueError, AttributeError):
+        raise argparse.ArgumentTypeError('This option must be in the format of "key=value"')
+
+    if key in ('branch', 'buildrequire_overrides', 'require_overrides', 'scmurl'):
+        raise argparse.ArgumentTypeError(
+            'The "{0}" optional argument is reserved to built-in arguments'.format(key))
+
+    return (key, value)
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 318e671..0cb9cfd 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -2191,10 +2191,10 @@ class TestModulesCli(CliTestCase):
             scopes=self.scopes,
             timeout=120)
 
-    @patch('requests.get')
-    def test_module_build_conflicting_keys(self, mock_get):
+    @patch('sys.stderr', new_callable=StringIO)
+    def test_module_build_conflicting_keys(self, stderr):
         """
-        Test a module build with optional arguments the conflict with other arguments
+        Test a module build with optional arguments that conflict with other arguments
         """
         cli_cmd = [
             'rpkg',
@@ -2205,17 +2205,14 @@ class TestModulesCli(CliTestCase):
             'master',
             '--optional', 'scmurl=git://pkgs.fedoraproject.org/modules/testmodule?#1234'
         ]
-        mock_get.return_value.ok = True
-        mock_get.return_value.json.return_value = {
-            'auth_method': 'oidc',
-            'api_version': 2
-        }
 
-        expected_error = 'The following optional arguments conflict with other arguments: scmurl'
         with patch('sys.argv', new=cli_cmd):
-            cli = self.new_cli()
-            with six.assertRaisesRegex(self, rpkgError, expected_error):
-                cli.module_build()
+            with self.assertRaises(SystemExit) as cm:
+                self.new_cli()
+
+        assert stderr.getvalue().strip().endswith(
+            'The "scmurl" optional argument is reserved to built-in arguments')
+        assert cm.exception.code == 2
 
     @patch('sys.stdout', new=StringIO())
     @patch('requests.get')