From 73bb7e91d92285fb7bb4a31bce68b8e49942339b Mon Sep 17 00:00:00 2001
From: Kamil Páral <kparal@redhat.com>
Date: Mar 21 2017 13:32:25 +0000
Subject: argparse: change --arch to be a single value instead of a string


This makes it easier to handle this value in formula and pass it to
other directives (like a bash directive), or modify it (append something
to it).

This is a substantial change. None of existing tasks seem to be making
use of multi-valued ${arch}. Generic tasks usually use 'all' arch value
in koji directive and process everything in one pass. Arch-specific
tasks will mostly want to handle just a single arch in a single pass.
That will also help us keep our sanity.

This patch is helpful for resolving T894 (making depcheck/rpmdeplint a
per-arch task).

As a convenience, all directives now support both str and a list of str
in `arch` parameters.

Differential Revision: https://phab.qa.fedoraproject.org/D1171

---

diff --git a/libtaskotron/directives/bodhi_directive.py b/libtaskotron/directives/bodhi_directive.py
index 17bad99..29baad7 100644
--- a/libtaskotron/directives/bodhi_directive.py
+++ b/libtaskotron/directives/bodhi_directive.py
@@ -11,6 +11,7 @@ from libtaskotron.exceptions import TaskotronDirectiveError
 import libtaskotron.ext.fedora.bodhi_utils as bodhi
 from libtaskotron.ext.fedora.koji_utils import KojiClient
 from libtaskotron import config
+from libtaskotron import exceptions as exc
 
 DOCUMENTATION = """
 module: bodhi_directive
@@ -34,11 +35,11 @@ parameters:
   arch:
     required: true
     description: |
-      a list of architectures for which to download RPMs.
+      an architecture (or a list of architectures) for which to download RPMs.
 
       Note: ``noarch`` RPMs are always automatically downloaded even when not
       requested, unless ``arch=[]`` and ``src=True``.
-    type: list of str
+    type: str or list of str
   src:
     required: false
     description: download also ``src`` RPM files
@@ -112,15 +113,23 @@ class BodhiDirective(BaseDirective):
 
     def process(self, params, arg_data):
         output_data = {}
-
         valid_actions = ['download']
-
         action = params['action']
 
         if action not in valid_actions:
             raise TaskotronDirectiveError('%s is not a valid command for bodhi directive'
                                  % action)
 
+        if 'arch' not in params:
+            detected_args = ', '.join(params.keys())
+            raise exc.TaskotronDirectiveError(
+                "The bodhi directive requires 'arch' as an argument. Detected "
+                "arguments: %s" % detected_args)
+
+        # convert str to list
+        if isinstance(params['arch'], basestring):
+            params['arch'] = [params['arch']]
+
         if action == 'download':
             if 'update_id' not in params or 'arch' not in params:
                 detected_args = ', '.join(params.keys())
diff --git a/libtaskotron/directives/koji_directive.py b/libtaskotron/directives/koji_directive.py
index 2064e09..1b528e3 100644
--- a/libtaskotron/directives/koji_directive.py
+++ b/libtaskotron/directives/koji_directive.py
@@ -28,24 +28,24 @@ parameters:
   arch:
     required: true
     description: |
-      a list of architectures for which to download RPMs for the requested build/tag. If you want
-      to download RPMs for all Taskotron-supported arches, use ``['all']``. If you don't want to
-      download any binary arch (i.e. you're only interested in SRPMs, which is controlled by `src`
-      option), use an empty list ``[]``.
+      an architecture (or a list of architectures) for which to download RPMs for the requested
+      build/tag. If you want to download RPMs for all Taskotron-supported arches, use ``'all'``.
+      If you don't want to download any binary arch (i.e. you're only interested in SRPMs, which is
+      controlled by `src` option), use an empty list ``[]``.
 
       Note: ``noarch`` RPMs are always automatically downloaded unless ``arch=[]``,
       or you specifically exclude them using ``arch_exclude``. Also, if you specify base arches
       (like ``i386``), all concrete binary arches for that base arch will be automatically added
       (e.g. ``i[3-6]86``).
-    type: list of str
+    type: str or list of str
     choices: [supported architectures, all]
   arch_exclude:
     required: false
     description: |
-      a list of architectures to exclude from downloading (overrides ``arch``
-      value). You can use it to exclude some specific archicture while using
-      ``arch=['all']``. Example: ``['armhfp', 'noarch']``
-    type: list of str
+      an architecture (or a list of architectures) to exclude from downloading (overrides ``arch``
+      value). You can use it to exclude some specific archicture while using ``arch='all'``.
+      Example: ``['armhfp', 'noarch']``
+    type: str or list of str
     choices: [supported architectures]
   build_log:
     required: false
@@ -106,7 +106,7 @@ src.rpm::
     koji:
         action: download
         koji_build: ${koji_build}
-        arch: ['all']
+        arch: all
         src: True
 
 Depcheck needs to download all builds in a specific Koji tag for the current
@@ -126,8 +126,8 @@ arches, but doesn't need ``noarch``::
     koji:
         action: download_latest_stable
         koji_build: ${koji_build}
-        arch: ['all']
-        arch_exclude: ['noarch']
+        arch: all
+        arch_exclude: noarch
         debuginfo: True
         target_dir: ${workdir}/stable
 
@@ -135,8 +135,8 @@ arches, but doesn't need ``noarch``::
     koji:
         action: download
         koji_build: ${koji_build}
-        arch: ['all']
-        arch_exclude: ['noarch']
+        arch: all
+        arch_exclude: noarch
         debuginfo: True
         target_dir: ${workdir}/update
 
@@ -180,6 +180,11 @@ class KojiDirective(BaseDirective):
                 "The koji directive requires 'arch' as an argument. Detected "
                 "arguments: %s" % detected_args)
 
+        # convert str to list
+        for param in ('arch', 'arch_exclude'):
+            if param in params and isinstance(params[param], basestring):
+                params[param] = [params[param]]
+
         arches = list(params['arch'])
         if arches and ('all' not in arches) and ('noarch' not in arches):
             arches.append('noarch')
diff --git a/libtaskotron/directives/mash_directive.py b/libtaskotron/directives/mash_directive.py
index f726a9d..56dbe47 100644
--- a/libtaskotron/directives/mash_directive.py
+++ b/libtaskotron/directives/mash_directive.py
@@ -29,8 +29,8 @@ parameters:
     type: str
   arch:
     required: true
-    description: list of architectures for which to create YUM metadata
-    type: list of str
+    description: an architecture (or a list of architectures) for which to create YUM metadata
+    type: str or list of str
   outdir:
     required: false
     description: absolute or relative path to an output directory, where
@@ -67,7 +67,6 @@ directory::
     - name: mash downloaded rpms
       mash:
           rpmdir: "${workdir}/downloaded_tag/"
-          dodelta: False
           arch: ${arch}
 """
 
@@ -160,6 +159,10 @@ class MashDirective(BaseDirective):
                 "The mash directive requires 'rpmdir' and 'arch' "
                 "arguments. Detected arguments: %s" % detected_args)
 
+        # convert str to list
+        if isinstance(params['arch'], basestring):
+            params['arch'] = [params['arch']]
+
         if 'dodelta' not in params:
             dodelta = False
         else:
diff --git a/libtaskotron/directives/yumrepoinfo_directive.py b/libtaskotron/directives/yumrepoinfo_directive.py
index 74c8a83..06807f8 100644
--- a/libtaskotron/directives/yumrepoinfo_directive.py
+++ b/libtaskotron/directives/yumrepoinfo_directive.py
@@ -24,10 +24,10 @@ parameters:
     type: str
   arch:
     required: true
-    description: list of architectures. In this list, all "meta architectures"
-      like ``all``, ``src`` and ``noarch`` are ignored, list of "real
-      architectures" is expected there (e.g. ``x86_64`` or ``i386``).
-    type: list of str
+    description: an architecure (or a list of architectures). All "meta architectures"
+      like ``all``, ``src`` and ``noarch`` are ignored, "real
+      architectures" are expected there instead (e.g. ``x86_64`` or ``i386``).
+    type: str or list of str
 returns: |
     A dictionary of dictionaries. Main key is ``koji_tag`` with value of
     dictionary that contains ``arch`` as key and URL for the respective
@@ -124,8 +124,11 @@ class YumrepoinfoDirective(BaseDirective):
             raise TaskotronDirectiveError("The yumrepoinfo directive requires "
                                           "koji_tag and arch arguments.")
 
-        arches = params['arch']
+        # convert str to list
+        if isinstance(params['arch'], basestring):
+            params['arch'] = [params['arch']]
 
+        arches = params['arch']
         processed_arches = [arch for arch in arches if arch in Arches.base]
 
         if len(processed_arches) == 0:
diff --git a/libtaskotron/ext/fedora/koji_utils.py b/libtaskotron/ext/fedora/koji_utils.py
index 6ddee56..ba7a0f4 100644
--- a/libtaskotron/ext/fedora/koji_utils.py
+++ b/libtaskotron/ext/fedora/koji_utils.py
@@ -197,10 +197,10 @@ class KojiClient(object):
         :rtype: list of str
         :raise TaskotronRemoteError: when the requested build doesn't exist
         '''
-        log.info('Querying Koji for a list of RPMS for: %s', nvr)
-
         arches = list(self._compute_arches(arches, arch_exclude, src))
 
+        log.info('Querying Koji for a list of RPMS for: %s (arches %s)', nvr, sorted(arches))
+
         if not arches:
             log.debug('Nothing to query for, the combination of `arches`, `arch_exclude` and '
                       '`src` ended up an empty list.')
diff --git a/libtaskotron/main.py b/libtaskotron/main.py
index c967b59..de983c7 100644
--- a/libtaskotron/main.py
+++ b/libtaskotron/main.py
@@ -30,10 +30,10 @@ def get_argparser():
     parser = argparse.ArgumentParser()
     parser.add_argument("task", help="task formula to run")
     parser.add_argument("-a", "--arch",
-                        choices=["i386", "x86_64", "armhfp", "noarch"],
-                        action='append',
-                        help="architecture specifying the item to be checked. If omitted, "
-                             "defaults to noarch.")
+                        choices=["i386", "x86_64", "armhfp", "noarch"], default='noarch',
+                        help="architecture specifying the item to be checked. 'noarch' value "
+                             'means an arch-independent task (if the task supports it, it can '
+                             'process all archs in a single go). [default: %(default)s]')
     parser.add_argument("-i", "--item", help="item to be checked")
     parser.add_argument("-t", "--type",
                         choices=check.ReportType.list(),
@@ -107,10 +107,6 @@ def process_args(raw_args):
     if args['type'] in check.ReportType.list():
         args[args['type']] = args['item']
 
-    # process arch
-    if args['arch'] is None:
-        args['arch'] = ['noarch']
-
     override = {}
     for var in args['override']:
         name, value = var.split('=', 1)
diff --git a/libtaskotron/taskformula.py b/libtaskotron/taskformula.py
index 58ba15f..a2f73e5 100644
--- a/libtaskotron/taskformula.py
+++ b/libtaskotron/taskformula.py
@@ -226,14 +226,11 @@ def devise_environment(formula, arg_data):
         log.debug("Environment/flavor not specified. Using default")
 
     if not env['arch']:
-        arches = [i for i in arg_data.get('arch', []) if i != 'noarch']
-
-        if arches and len(arches) == 1:
-            env['arch'] = arches[0]
-            log.debug("Environment/arch inferred from %r to %r" %
-                      (arg_data.get('arch', []), env['arch']))
+        arch = arg_data.get('arch')
+        if not arch or arch == 'noarch':
+            log.warn("Environment/arch can not be inferred from %r. Using default." % arch)
         else:
-            log.warn("Environment/arch can not be inferred from %r. Using default." %
-                     arg_data.get('arch', []))
+            env['arch'] = arch
+            log.debug("Environment/arch inferred from %r to %r" % (arch, env['arch']))
 
     return env
diff --git a/testing/functest_mash_directive.py b/testing/functest_mash_directive.py
index f7f5fb3..d0d07f5 100644
--- a/testing/functest_mash_directive.py
+++ b/testing/functest_mash_directive.py
@@ -47,7 +47,7 @@ class TestMashDirective(object):
     def test_mash_arch_i686(self):
         params = {
             'rpmdir': self.rpmdir,
-            'arch': ['i386']
+            'arch': 'i386'
         }
         directive = mash_directive.MashDirective()
         before = os.listdir(self.rpmdir)
@@ -61,6 +61,19 @@ class TestMashDirective(object):
     def test_mash_arch_x86_64(self):
         params = {
             'rpmdir': self.rpmdir,
+            'arch': 'x86_64'
+        }
+        directive = mash_directive.MashDirective()
+        before = os.listdir(self.rpmdir)
+        directive.process(params, {})
+        after = os.listdir(self.rpmdir)
+
+        assert (self.rpms[0] in before and self.rpms[0] in after and
+                self.rpms[1] in before and self.rpms[1] not in after)
+
+    def test_mash_arch_as_list(self):
+        params = {
+            'rpmdir': self.rpmdir,
             'arch': ['x86_64']
         }
         directive = mash_directive.MashDirective()
@@ -76,7 +89,7 @@ class TestMashDirective(object):
         params = {
             'rpmdir': self.rpmdir,
             'outdir': outdir_path,
-            'arch': ['i386']
+            'arch': 'i386'
         }
 
         directive = mash_directive.MashDirective()
@@ -89,7 +102,7 @@ class TestMashDirective(object):
     def test_repodata_in_rpmdir(self):
         params = {
             'rpmdir': self.rpmdir,
-            'arch': ['x86_64']
+            'arch': 'x86_64'
         }
         directive = mash_directive.MashDirective()
         directive.process(params, {})
@@ -102,7 +115,7 @@ class TestMashDirective(object):
         params = {
             'rpmdir': self.rpmdir,
             'outdir': outdir,
-            'arch': ['x86_64']
+            'arch': 'x86_64'
         }
         directive = mash_directive.MashDirective()
         directive.process(params, {})
diff --git a/testing/functest_yumrepoinfo_directive.py b/testing/functest_yumrepoinfo_directive.py
index 436dd20..9f744b3 100644
--- a/testing/functest_yumrepoinfo_directive.py
+++ b/testing/functest_yumrepoinfo_directive.py
@@ -27,7 +27,7 @@ class TestYumrepoinfoDirective(object):
 
     def test_missing_kojitag(self):
         directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
-        ref_input = {"arch": ["x86_64"]}
+        ref_input = {"arch": "x86_64"}
 
         with pytest.raises(TaskotronDirectiveError):
             directive.process(ref_input, None)
@@ -41,7 +41,7 @@ class TestYumrepoinfoDirective(object):
 
     def test_pending(self):
         directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
-        ref_input = {"koji_tag": "f20-pending", "arch": ["x86_64"]}
+        ref_input = {"koji_tag": "f20-pending", "arch": "x86_64"}
 
         output = directive.process(ref_input, None)
 
@@ -49,7 +49,7 @@ class TestYumrepoinfoDirective(object):
 
     def test_rawhide(self):
         directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
-        ref_input = {"koji_tag": "rawhide", "arch": ["x86_64"]}
+        ref_input = {"koji_tag": "rawhide", "arch": "x86_64"}
 
         output = directive.process(ref_input, None)
 
@@ -65,7 +65,7 @@ class TestYumrepoinfoDirective(object):
     def test_repo_path(self):
 
         directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
-        ref_input = {"koji_tag": "f20-updates", "arch": ["x86_64"]}
+        ref_input = {"koji_tag": "f20-updates", "arch": "x86_64"}
 
         output = directive.process(ref_input, None)
 
@@ -78,7 +78,7 @@ class TestYumrepoinfoDirective(object):
         """Make sure that the arch passed in as an arg is used to create the
         yumrepoinfo object instead of falling back to the default system arch"""
         ref_arch = 'i386'
-        repoinfo = yumrepoinfo.YumRepoInfo(filelist=[], arch=['x86_64'])
+        repoinfo = yumrepoinfo.YumRepoInfo(filelist=[], arch='x86_64')
         repoinfo.parser.readfp(StringIO.StringIO(TEST_CONF))
 
         stub_getrepoinfo = Dingus(return_value=repoinfo)
@@ -87,7 +87,7 @@ class TestYumrepoinfoDirective(object):
         # don't set the repoinfo object, we've stubbed out the code that would
         # hit the filesystem, so it's not a risk here
         directive = yumrepoinfo_directive.YumrepoinfoDirective()
-        ref_input = {"koji_tag": "f20-updates", "arch": [ref_arch]}
+        ref_input = {"koji_tag": "f20-updates", "arch": ref_arch}
 
         directive.process(ref_input, None)
 
@@ -103,6 +103,17 @@ class TestYumrepoinfoDirective(object):
 
     def test_one_base_arch(self):
         directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
+        ref_input = {"koji_tag": "f20", "arch": "x86_64"}
+
+        output = directive.process(ref_input, None)
+
+        assert output == {
+            "f20": {"x86_64": "http://download.fedoraproject.org/pub/fedora/linux/releases/20/"
+                              "Everything/x86_64/os"}
+        }
+
+    def test_one_arch_as_list(self):
+        directive = yumrepoinfo_directive.YumrepoinfoDirective(filelist=[self.temp_conf.strpath])
         ref_input = {"koji_tag": "f20", "arch": ["x86_64"]}
 
         output = directive.process(ref_input, None)
diff --git a/testing/test_bodhi_directive.py b/testing/test_bodhi_directive.py
index 74b3349..2353832 100644
--- a/testing/test_bodhi_directive.py
+++ b/testing/test_bodhi_directive.py
@@ -16,7 +16,7 @@ class TestBodhiDownloads():
 
     def setup_method(self, method):
         '''Run this before every test invocation'''
-        self.ref_arch = ['x86_64']
+        self.ref_arch = 'x86_64'
         self.ref_update_id = 'FEDORA-1234-56789'
         self.ref_workdir = '/var/tmp/foo'
         self.ref_taskfile = '/foo/bar/task.yml'
@@ -61,7 +61,7 @@ class TestBodhiDownloads():
 
     def test_action_download_all_archs(self):
         '''Test download when all arches are demanded'''
-        self.ref_arch = ['all']
+        self.ref_arch = 'all'
         self.ref_input = {'action': 'download',
                           'arch': self.ref_arch,
                           'update_id': self.ref_update_id
@@ -126,7 +126,7 @@ class TestBodhiDownloads():
 
     def test_action_download_added_noarch(self):
         '''Test whether noarch is automaticaly added'''
-        self.ref_arch = ['i386']
+        self.ref_arch = 'i386'
         self.ref_input = {'action': 'download',
                           'arch': self.ref_arch,
                           'update_id': self.ref_update_id
diff --git a/testing/test_koji_directive.py b/testing/test_koji_directive.py
index 201b3cf..5a43e2d 100644
--- a/testing/test_koji_directive.py
+++ b/testing/test_koji_directive.py
@@ -19,7 +19,7 @@ class TestKojiDirective():
 
         self.ref_nvr = 'foo-1.2-3.fc99'
         self.ref_previous_nvr = 'foo-1.2-2.fc99'
-        self.ref_arch = ['noarch']
+        self.ref_arch = 'noarch'
         self.ref_tag = 'tagfoo'
         self.ref_name = 'foo'
         self.ref_version = '1.2'
@@ -116,8 +116,8 @@ class TestKojiDirective():
 
     @pytest.mark.parametrize('action', ACTIONS)
     def test_parse_multiple_arches(self, action):
-        ref_arch = ['noarch', 'i386']
-        ref_input = {'action': action, 'arch': ref_arch,
+        self.ref_arch = ['noarch', 'i386']
+        ref_input = {'action': action, 'arch': self.ref_arch,
                      'koji_build': self.ref_nvr, 'koji_tag': self.ref_tag}
 
         stub_koji = Dingus('koji_utils')
@@ -128,7 +128,7 @@ class TestKojiDirective():
         assert len(getrpm_calls) >= 1
         for call in getrpm_calls:
             requested_arches = call[2]['arches']
-            assert requested_arches == ref_arch
+            assert requested_arches == self.ref_arch
 
     @pytest.mark.parametrize('action', ACTIONS)
     def test_parse_single_arch(self, action):
@@ -144,11 +144,27 @@ class TestKojiDirective():
         assert len(getrpm_calls) >= 1
         for call in getrpm_calls:
             requested_arches = call[2]['arches']
-            assert requested_arches == self.ref_arch
+            assert requested_arches == [self.ref_arch]
+
+    @pytest.mark.parametrize('action', ACTIONS)
+    def test_parse_single_arch_as_list(self, action):
+        ref_input = {'action': action, 'arch': [self.ref_arch],
+                     'koji_build': self.ref_nvr, 'koji_tag': self.ref_tag}
+
+        stub_koji = Dingus('koji_utils')
+
+        test_helper = koji_directive.KojiDirective(stub_koji)
+        test_helper.process(ref_input, self.ref_arg_data)
+
+        getrpm_calls = [call for call in stub_koji.calls() if call[0] in self.koji_calls]
+        assert len(getrpm_calls) >= 1
+        for call in getrpm_calls:
+            requested_arches = call[2]['arches']
+            assert requested_arches == [self.ref_arch]
 
     @pytest.mark.parametrize('action', ACTIONS)
     def test_add_noarch(self, action):
-        ref_input = {'action': action, 'arch': ['x86_64'],
+        ref_input = {'action': action, 'arch': 'x86_64',
                      'koji_build': self.ref_nvr, 'koji_tag': self.ref_tag}
 
         stub_koji = Dingus()
@@ -165,7 +181,7 @@ class TestKojiDirective():
 
     @pytest.mark.parametrize('action', ACTIONS)
     def test_exclude_arch(self, action):
-        ref_input = {'action': action, 'arch': ['all'], 'arch_exclude': ['x86_64', 'noarch'],
+        ref_input = {'action': action, 'arch': 'all', 'arch_exclude': ['x86_64', 'noarch'],
                      'koji_build': self.ref_nvr, 'koji_tag': self.ref_tag}
 
         stub_koji = Dingus()
diff --git a/testing/test_main.py b/testing/test_main.py
index b737192..e70ef58 100644
--- a/testing/test_main.py
+++ b/testing/test_main.py
@@ -49,11 +49,6 @@ class TestProcessArgs():
         test_args = main.process_args(self.ref_input)
         assert test_args['arch'] == self.ref_input['arch']
 
-    def test_no_arch(self):
-        self.ref_input['arch'] = None
-        test_args = main.process_args(self.ref_input)
-        assert test_args['arch'] == ['noarch']
-
     def test_orig_args(self):
         test_args = main.process_args(self.ref_input)
         assert test_args['_orig_args'] == self.ref_input
diff --git a/testing/test_taskformula.py b/testing/test_taskformula.py
index 39937e6..b5b8b77 100644
--- a/testing/test_taskformula.py
+++ b/testing/test_taskformula.py
@@ -226,23 +226,14 @@ class TestDeviseEnvironment(object):
         assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': None}
 
     def test_infer_from_arch(self):
-        arg_data = {"arch": []}
+        arg_data = {"arch": None}
         ret = taskformula.devise_environment({'environment': {}}, arg_data)
         assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': None}
 
-        arg_data = {"arch": ['noarch']}
+        arg_data = {"arch": 'noarch'}
         ret = taskformula.devise_environment({'environment': {}}, arg_data)
         assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': None}
 
-        arg_data = {"arch": ['i386']}
+        arg_data = {"arch": 'i386'}
         ret = taskformula.devise_environment({'environment': {}}, arg_data)
         assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': 'i386'}
-
-    def test_infer_from_multiple_arches(self):
-        arg_data = {"arch": ['i386', 'noarch']}
-        ret = taskformula.devise_environment({'environment': {}}, arg_data)
-        assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': 'i386'}
-
-        arg_data = {"arch": ['i386', 'x86_64']}
-        ret = taskformula.devise_environment({'environment': {}}, arg_data)
-        assert ret == {'distro': None, 'release': None, 'flavor': None, 'arch': None}