Fix download-task all files in build/buildArch method tasks

cli/koji_cli/commands.py

@@ -6878,7 +6878,9 @@ def anon_handle_download_logs(options, session, args):
 
 def anon_handle_download_task(options, session, args):
     "[download] Download the output of a build task"
-    usage = "usage: %prog download-task <task_id>"
+    usage = "usage: %prog download-task <task_id>\n" \
+            "Default behavior without --all option downloads .rpm files only for build " \
+            "and buildArch tasks.\n"
     parser = OptionParser(usage=get_usage_str(usage))
     parser.add_option("--arch", dest="arches", metavar="ARCH", action="append", default=[],
                       help="Only download packages for this arch (may be used multiple times), "
@@ -6929,67 +6931,69 @@ def anon_handle_download_task(options, session, args):
     if not suboptions.parentonly:
         list_tasks.extend(session.getTaskChildren(base_task_id))
 
-    # support file types
+    required_tasks = {}
-    expected_types = ['rpm', 'log']
+    for task in list_tasks:
-    for type in session.getArchiveTypes():
+        if task["id"] not in required_tasks:
-        expected_types.extend(type['extensions'].split(' '))
+            required_tasks[task["id"]] = task
+
+    for task_id in required_tasks:
+        if required_tasks[task_id]["state"] != koji.TASK_STATES.get("CLOSED"):
+            if task_id == base_task_id:
+                error("Task %d has not finished yet." % task_id)
+            else:
+                error("Child task %d has not finished yet." % task_id)
 
     # get files for download
     downloads = []
     build_methods_list = ['buildArch', 'build']
+    rpm_file_types = ['rpm', 'src.rpm']
     for task in list_tasks:
         taskarch = task['arch']
         task_id = str(task['id'])
         if len(suboptions.arches) == 0 or taskarch in suboptions.arches:
             files = list_task_output_all_volumes(session, task["id"])
-            filetype = None
             for filename in files:
                 if filename.endswith('src.rpm'):
                     filetype = 'src.rpm'
                 else:
-                    for ft in expected_types:
+                    filetype = filename.rsplit('.', 1)[1]
-                        if filename.endswith('.%s' % ft):
+                if suboptions.all and not (task['method'] in build_methods_list and
-                            filetype = ft
+                                           filetype in rpm_file_types):
-                            break
+                    if filetype != 'log':
-                if not filetype:
-                    warn('Unsupported file type for download-task: %s' % filename)
-                else:
-                    if suboptions.all and task['method'] not in build_methods_list:
-                        if filetype != 'log':
-                            for volume in files[filename]:
-                                if suboptions.dirpertask:
-                                    new_filename = '%s/%s' % (task_id, filename)
-                                else:
-                                    if taskarch not in filename and filetype != 'src.rpm':
-                                        part_filename = filename[:-len('.%s' % filetype)]
-                                        new_filename = "%s.%s.%s" % (part_filename,
-                                                                     taskarch, filetype)
-                                    else:
-                                        new_filename = filename
-                                downloads.append((task, filename, volume, new_filename, task_id))
-                    elif task['method'] in build_methods_list:
-                        if filetype in ['rpm', 'src.rpm']:
-                            filearch = filename.split(".")[-2]
-                            for volume in files[filename]:
-                                if len(suboptions.arches) == 0 or filearch in suboptions.arches:
-                                    if suboptions.dirpertask:
-                                        new_filename = '%s/%s' % (task_id, filename)
-                                    else:
-                                        new_filename = filename
-                                    downloads.append((task, filename, volume, new_filename,
-                                                      task_id))
-
-                    if filetype == 'log' and suboptions.logs:
                         for volume in files[filename]:
                             if suboptions.dirpertask:
                                 new_filename = '%s/%s' % (task_id, filename)
                             else:
-                                if taskarch not in filename:
+                                if taskarch not in filename and filetype != 'src.rpm':
-                                    part_filename = filename[:-len('.log')]
+                                    part_filename = filename[:-len('.%s' % filetype)]
-                                    new_filename = "%s.%s.log" % (part_filename, taskarch)
+                                    new_filename = "%s.%s.%s" % (part_filename,
+                                                                 taskarch, filetype)
                                 else:
                                     new_filename = filename
                             downloads.append((task, filename, volume, new_filename, task_id))
+                elif task['method'] in build_methods_list:
+                    if filetype in rpm_file_types:
+                        filearch = filename.split(".")[-2]
+                        for volume in files[filename]:
+                            if len(suboptions.arches) == 0 or filearch in suboptions.arches:
+                                if suboptions.dirpertask:
+                                    new_filename = '%s/%s' % (task_id, filename)
+                                else:
+                                    new_filename = filename
+                                downloads.append((task, filename, volume, new_filename,
+                                                  task_id))
+
+                if filetype == 'log' and suboptions.logs:
+                    for volume in files[filename]:
+                        if suboptions.dirpertask:
+                            new_filename = '%s/%s' % (task_id, filename)
+                        else:
+                            if taskarch not in filename:
+                                part_filename = filename[:-len('.log')]
+                                new_filename = "%s.%s.log" % (part_filename, taskarch)
+                            else:
+                                new_filename = filename
+                        downloads.append((task, filename, volume, new_filename, task_id))
 
     if len(downloads) == 0:
         print("No files for download found.")

tests/test_cli/test_download_task.py

@@ -57,6 +57,8 @@ class TestDownloadTask(utils.CliTestCase):
         self.parent_task_info = {'id': self.parent_task_id, 'method': 'buildArch',
                                  'arch': 'taskarch', 'state': 2, 'parent': None}
         self.error_format = """Usage: %s download-task <task_id>
+Default behavior without --all option downloads .rpm files only for build and buildArch tasks.
+
 (Specify the --help global option for a list of other help options)
 
 %s: error: {message}
@@ -257,8 +259,7 @@ class TestDownloadTask(utils.CliTestCase):
         self.ensure_connection.assert_called_once_with(self.session, self.options)
         self.session.getTaskInfo.assert_called_once_with(self.parent_task_id)
         self.session.getTaskChildren.assert_called_once_with(self.parent_task_id)
-        self.list_task_output_all_volumes.assert_called_once_with(
+        self.list_task_output_all_volumes.assert_not_called()
-            self.session, self.parent_task_id)
         self.download_file.assert_not_called()
 
     def test_handle_download_child_not_finished(self):
@@ -285,8 +286,7 @@ class TestDownloadTask(utils.CliTestCase):
         self.ensure_connection.assert_called_once_with(self.session, self.options)
         self.session.getTaskInfo.assert_called_once_with(self.parent_task_id)
         self.session.getTaskChildren.assert_called_once_with(self.parent_task_id)
-        self.list_task_output_all_volumes.assert_has_calls(
+        self.list_task_output_all_volumes.assert_not_called()
-            [mock.call(self.session, self.parent_task_id), mock.call(self.session, 22222)])
         self.download_file.assert_not_called()
 
     def test_handle_download_invalid_file_name(self):
@@ -321,6 +321,8 @@ class TestDownloadTask(utils.CliTestCase):
         self.assertExitCode(ex, 0)
         actual = self.stdout.getvalue()
         expected = """Usage: %s download-task <task_id>
+Default behavior without --all option downloads .rpm files only for build and buildArch tasks.
+
 (Specify the --help global option for a list of other help options)
 
 Options:
@@ -447,7 +449,7 @@ Options:
                                                      ]
         self.list_task_output_all_volumes.side_effect = [
             {'somerpm.src.rpm': ['DEFAULT', 'vol1']},
-            {'somerpm.x86_64.rpm': ['DEFAULT', 'vol2']},
+            {'somerpm.json': ['DEFAULT', 'vol2']},
             {'somerpm.noarch.rpm': ['vol3'],
              'somelog.log': ['DEFAULT', 'vol1']},
         ]
@@ -468,10 +470,10 @@ Options:
             call(self.session, 33333),
             call(self.session, 55555)])
         self.assertListEqual(self.download_file.mock_calls, [
-            call('https://topurl/work/tasks/3333/33333/somerpm.x86_64.rpm',
+            call('https://topurl/work/tasks/3333/33333/somerpm.json',
-                 'somerpm.x86_64.rpm', quiet=None, noprogress=None, size=3, num=1),
+                 'somerpm.x86_64.json', quiet=None, noprogress=None, size=3, num=1),
-            call('https://topurl/vol/vol2/work/tasks/3333/33333/somerpm.x86_64.rpm',
+            call('https://topurl/vol/vol2/work/tasks/3333/33333/somerpm.json',
-                 'vol2/somerpm.x86_64.rpm', quiet=None, noprogress=None, size=3, num=2),
+                 'vol2/somerpm.x86_64.json', quiet=None, noprogress=None, size=3, num=2),
             call('https://topurl/vol/vol3/work/tasks/5555/55555/somerpm.noarch.rpm',
                  'vol3/somerpm.noarch.rpm', quiet=None, noprogress=None, size=3, num=3),
         ])
@@ -718,3 +720,36 @@ Options:
             call(self.session, 44444),
             call(self.session, 55555)])
         self.assertListEqual(self.download_file.mock_calls, [])
+
+    def test_handle_download_task_without_all_json_not_downloaded(self):
+        args = [str(self.parent_task_id)]
+        self.session.getTaskInfo.return_value = self.parent_task_info
+        self.session.getTaskChildren.return_value = []
+        self.list_task_output_all_volumes.return_value = {
+            'somerpm.src.rpm': ['DEFAULT', 'vol1'],
+            'somerpm.x86_64.rpm': ['DEFAULT', 'vol2'],
+            'somerpm.noarch.rpm': ['vol3'],
+            'somelog.log': ['DEFAULT', 'vol1'],
+            'somefile.json': ['DEFAULT', 'vol1'],
+        }
+
+        calls = self.gen_calls(self.list_task_output_all_volumes.return_value,
+                               'https://topurl/%swork/tasks/3333/123333/%s',
+                               ['somelog.log', 'somefile.json'])
+
+        # Run it and check immediate output
+        # args: task_id
+        # expected: success
+        rv = anon_handle_download_task(self.options, self.session, args)
+
+        actual = self.stdout.getvalue()
+        expected = ''
+        self.assertMultiLineEqual(actual, expected)
+        # Finally, assert that things were called as we expected.
+        self.ensure_connection.assert_called_once_with(self.session, self.options)
+        self.session.getTaskInfo.assert_called_once_with(self.parent_task_id)
+        self.session.getTaskChildren.assert_called_once_with(self.parent_task_id)
+        self.list_task_output_all_volumes.assert_called_once_with(self.session,
+                                                                  self.parent_task_id)
+        self.assertListEqual(self.download_file.mock_calls, calls)
+        self.assertIsNone(rv)