Fail pack download when any requested pack fails (#4620)

CHANGELOG.rst

@@ -26,6 +26,9 @@ Fixed
 * Fix utf-8 encode before checking paramter max size #6352
 * Fix stuck running workflow tasks #6398 (by @guzzijones12@gmail.com)
 * Fixed action-alias help API returning 500 when RBAC enabled. #6414 (by @nzlosh)
+* Fix ``st2 pack install`` silently ignoring packs which fail to download (e.g. non-existent
+  packs) when installing multiple packs at once. The ``packs.download`` action now fails and
+  lists the packs which could not be downloaded. #4620
 
 Changed
 ~~~~~~~

contrib/packs/actions/pack_mgmt/download.py

@@ -107,23 +107,40 @@ def run(
 
     @staticmethod
     def _validate_result(result, repo_url):
-        atleast_one_success = False
+        """
+        Validate the download results and raise if downloading any of the requested
+        packs failed.
+
+        Previously an error was only raised if *all* the packs failed to download which
+        meant that "st2 pack install <existing> <non-existent>" silently ignored the
+        non-existent pack and reported success (see #4620).
+        """
         sanitized_result = {}
+        failed_packs = []
 
-        for k, v in six.iteritems(result):
-            atleast_one_success |= v[0]
-            sanitized_result[k] = v[1]
+        for pack, (success, message) in six.iteritems(result):
+            sanitized_result[pack] = message
 
-        if not atleast_one_success:
+            if not success:
+                failed_packs.append(pack)
+
+        if failed_packs:
             message_list = []
-            message_list.append(
-                'The pack has not been downloaded from "%s".\n' % (repo_url)
-            )
+
+            if len(result) == 1:
+                message_list.append(
+                    'The pack has not been downloaded from "%s".\n' % (repo_url)
+                )
+            else:
+                message_list.append(
+                    "The following packs have not been downloaded: %s\n"
+                    % (", ".join(failed_packs))
+                )
+
             message_list.append("Errors:")
 
-            for pack, value in result.items():
-                success, error = value
-                message_list.append(error)
+            for pack in failed_packs:
+                message_list.append(result[pack][1])
 
             message = "\n".join(message_list)
             raise Exception(message)

contrib/packs/tests/test_action_download.py

@@ -236,6 +236,35 @@ def test_run_pack_download_multiple_packs(self):
         self.assertTrue(os.path.isfile(os.path.join(self.repo_base, "test/pack.yaml")))
         self.assertTrue(os.path.isfile(os.path.join(self.repo_base, "test2/pack.yaml")))
 
+    def test_run_pack_download_multiple_packs_one_does_not_exist(self):
+        # Regression test for #4620 - a non-existent pack must not be silently ignored
+        # when other packs are downloaded successfully
+        action = self.get_action_instance()
+
+        for packs in (["test", "doesnotexist"], ["doesnotexist", "test"]):
+            expected_msg = (
+                r"The following packs have not been downloaded: doesnotexist"
+                r"(.|\n)*No record of the \"doesnotexist\" pack in the index"
+            )
+            self.assertRaisesRegex(
+                Exception,
+                expected_msg,
+                action.run,
+                packs=packs,
+                abs_repo_base=self.repo_base,
+                force=True,
+            )
+
+    def test_run_pack_download_single_pack_does_not_exist(self):
+        action = self.get_action_instance()
+        self.assertRaisesRegex(
+            Exception,
+            r"The pack has not been downloaded from \"None\"",
+            action.run,
+            packs=["doesnotexist"],
+            abs_repo_base=self.repo_base,
+        )
+
     @mock.patch.object(Repo, "clone_from")
     def test_run_pack_download_error(self, clone_from):
         clone_from.side_effect = Exception(