From 7dfd788faad34855b65927f41f05002d57bebc11 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 01 2024 20:18:14 +0000
Subject: [PATCH 1/2] download-logs: prefer getBuildLogs for builds


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index a44dcaa..ebd9e7b 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -7035,25 +7035,25 @@ def anon_handle_download_logs(options, session, args):
                 save_logs(child_task['id'], match, task_log_dir, recurse)
 
     ensure_connection(session, options)
-    task_id = None
-    build_id = None
     for arg in args:
+        task_id = None
+        build_id = None
         if suboptions.nvr:
             suboptions.recurse = True
             binfo = session.getBuild(arg)
             if binfo is None:
                 error("There is no build with n-v-r: %s" % arg)
-            if binfo.get('task_id'):
-                task_id = binfo['task_id']
-                sys.stdout.write("Using task ID: %s\n" % task_id)
-            elif binfo.get('build_id'):
+            if binfo.get('build_id'):
                 build_id = binfo['build_id']
                 sys.stdout.write("Using build ID: %s\n" % build_id)
+            elif binfo.get('task_id'):
+                task_id = binfo['task_id']
+                sys.stdout.write("Using task ID: %s\n" % task_id)
         else:
             try:
                 task_id = int(arg)
             except ValueError:
-                error("Task id must be number: %r" % arg)
+                error("Task id must be a number: %r" % arg)
         if task_id:
             save_logs(task_id, suboptions.match, suboptions.dir, suboptions.recurse)
         elif build_id:
diff --git a/tests/test_cli/test_download_logs.py b/tests/test_cli/test_download_logs.py
index ca7bb64..9b4b1ab 100644
--- a/tests/test_cli/test_download_logs.py
+++ b/tests/test_cli/test_download_logs.py
@@ -56,7 +56,7 @@ Note this command only downloads task logs, not build logs.
             anon_handle_download_logs,
             self.options, self.session, [task_id],
             stdout='',
-            stderr='Task id must be number: %r\n' % task_id,
+            stderr='Task id must be a number: %r\n' % task_id,
             activate_session=None,
             exit_code=1
         )

From 4ea3dcf96b3c13001828eb1da582c3c3c4fde99b Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Aug 13 2024 18:44:22 +0000
Subject: [PATCH 2/2] fall back to task files for non-complete builds


---

diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py
index ebd9e7b..b91549e 100644
--- a/cli/koji_cli/commands.py
+++ b/cli/koji_cli/commands.py
@@ -7043,12 +7043,18 @@ def anon_handle_download_logs(options, session, args):
             binfo = session.getBuild(arg)
             if binfo is None:
                 error("There is no build with n-v-r: %s" % arg)
-            if binfo.get('build_id'):
+            b_state = koji.BUILD_STATES[binfo['state']]
+            # getBuildLogs will only work for complete builds
+            if b_state == 'COMPLETE':
                 build_id = binfo['build_id']
                 sys.stdout.write("Using build ID: %s\n" % build_id)
             elif binfo.get('task_id'):
+                # try falling back to the task logs
                 task_id = binfo['task_id']
+                sys.stdout.write("Build %s is %s\n" % (arg, b_state))
                 sys.stdout.write("Using task ID: %s\n" % task_id)
+            else:
+                sys.stdout.write("Unable to download build %s (state=%s)\n" % (arg, b_state))
         else:
             try:
                 task_id = int(arg)
diff --git a/tests/test_cli/test_download_logs.py b/tests/test_cli/test_download_logs.py
index 9b4b1ab..a5d9033 100644
--- a/tests/test_cli/test_download_logs.py
+++ b/tests/test_cli/test_download_logs.py
@@ -3,6 +3,8 @@ from __future__ import absolute_import
 import mock
 import six
 
+import koji
+
 from koji_cli.commands import anon_handle_download_logs
 from . import utils
 
@@ -38,6 +40,7 @@ Note this command only downloads task logs, not build logs.
 """ % (self.progname, self.progname, self.progname)
         self.nvr = 'bash-1.2.3-f26'
         self.task_id = 123456
+        self.build_id = 232323
 
         self.builtin_open = None
         if six.PY2:
@@ -99,7 +102,31 @@ Note this command only downloads task logs, not build logs.
         self.session.getTaskChildren.assert_not_called()
 
     def test_anon_handle_download_logs_nvr(self):
-        self.session.getBuild.return_value = {'task_id': self.task_id}
+        self.session.getBuild.return_value = {'task_id': self.task_id,
+                                              'build_id': self.build_id,
+                                              'state': koji.BUILD_STATES['COMPLETE']}
+        self.session.getBuildLogs.return_value = [
+            {'dir': 'noarch', 'name': 'test.log', 'path': 'path/to/test.log'},
+        ]
+        rv = anon_handle_download_logs(self.options, self.session, ['--nvr', self.nvr])
+        actual = self.stdout.getvalue()
+        expected = 'Using build ID: %s\n' % self.build_id
+        self.assertMultiLineEqual(actual, expected)
+        self.assertIsNone(rv)
+
+        self.session.getBuild.assert_called_once_with(self.nvr)
+        self.session.getBuildLogs.assert_called_once_with(self.build_id)
+        self.session.getTaskInfo.assert_not_called()
+        self.session.downloadTaskOutput.assert_not_called()
+        self.session.getTaskChildren.assert_not_called()
+        self.download_file.assert_called_once()
+
+    def test_anon_handle_download_logs_nvr_with_task_id(self):
+        # for a failed build with a task id we should fall back to fetching task logs
+        self.session.getBuild.return_value = {'build_id': self.build_id,
+                                              'task_id': self.task_id,
+                                              'nvr': self.nvr,
+                                              'state': koji.BUILD_STATES['FAILED']}
         self.session.getTaskInfo.return_value = {
             'arch': 'x86_64',
             'state': 'CLOSED',
@@ -109,7 +136,7 @@ Note this command only downloads task logs, not build logs.
 
         rv = anon_handle_download_logs(self.options, self.session, ['--nvr', self.nvr])
         actual = self.stdout.getvalue()
-        expected = 'Using task ID: %s\n' % self.task_id
+        expected = 'Build %s is FAILED\nUsing task ID: %s\n' % (self.nvr, self.task_id)
         self.assertMultiLineEqual(actual, expected)
         self.assertIsNone(rv)
 
@@ -118,11 +145,14 @@ Note this command only downloads task logs, not build logs.
         self.session.downloadTaskOutput.assert_not_called()
         self.session.getTaskChildren.assert_has_calls([mock.call(self.task_id), mock.call(23), ])
 
+
     def test_anon_handle_download_logs_nvr_without_task_id(self):
-        self.session.getBuild.return_value = {'build_id': 1, 'nvr': self.nvr}
+        self.session.getBuild.return_value = {'build_id': self.build_id,
+                                              'nvr': self.nvr,
+                                              'state': koji.BUILD_STATES['FAILED']}
         rv = anon_handle_download_logs(self.options, self.session, ['--nvr', self.nvr])
         actual = self.stdout.getvalue()
-        expected = 'Using build ID: 1\n'
+        expected = 'Unable to download build %s (state=FAILED)\n' % self.nvr
         self.assertMultiLineEqual(actual, expected)
         self.assertIsNone(rv)
 

