Fix wrongful "--image-repositories" opt validation when all functions…

… are valid ECR URL (#2934)
aws · Aug 10, 2023 · 2b7c9d2 · 2b7c9d2
1 parent cae8e2d
commit 2b7c9d2
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 56 deletions.
diff --git a/samcli/lib/cli_validation/image_repository_validation.py b/samcli/lib/cli_validation/image_repository_validation.py
@@ -5,7 +5,6 @@
 import click
 
 from samcli.commands._utils.option_validator import Validator
-from samcli.commands._utils.template import get_template_artifacts_format
 from samcli.lib.providers.provider import (
     ResourceIdentifier,
     get_resource_full_path_by_id,
@@ -49,16 +48,6 @@ def wrapped(*args, **kwargs):
                 or ctx.params.get("template", False)
             )
 
-            # Check if `--image-repository`, `--image-repositories`, or `--resolve-image-repos` are required by
-            # looking for resources that have an IMAGE based packagetype.
-
-            required = any(
-                [
-                    _template_artifact == IMAGE
-                    for _template_artifact in get_template_artifacts_format(template_file=template_file)
-                ]
-            )
-
             available_options = "'--image-repositories', '--image-repository'"
             if support_resolve_image_repos:
                 available_options += ", '--resolve-image-repos'"
@@ -82,16 +71,6 @@ def wrapped(*args, **kwargs):
                         "Do you have multiple specified in the command or in a configuration file?",
                     ),
                 ),
-                Validator(
-                    validation_function=lambda: not guided
-                    and not (image_repository or image_repositories or resolve_image_repos)
-                    and required,
-                    exception=click.BadOptionUsage(
-                        option_name="--image-repositories",
-                        ctx=ctx,
-                        message=f"Missing option {available_options}",
-                    ),
-                ),
                 Validator(
                     validation_function=lambda: not guided
                     and (
@@ -103,6 +82,17 @@ def wrapped(*args, **kwargs):
                         option_name="--image-repositories", ctx=ctx, message=image_repos_error_msg
                     ),
                 ),
+                Validator(
+                    validation_function=lambda: not guided
+                    and not image_repositories
+                    and not _is_all_image_funcs_provided(template_file, {}, parameters_overrides)
+                    and not (image_repository or resolve_image_repos),
+                    exception=click.BadOptionUsage(
+                        option_name="--image-repositories",
+                        ctx=ctx,
+                        message=f"Missing option {available_options}",
+                    ),
+                ),
             ]
             for validator in validators:
                 validator.validate()

diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py
@@ -523,18 +523,15 @@ def test_local_start_lambda(self, do_cli_mock):
             )
 
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     @patch("samcli.commands._utils.options.get_template_artifacts_format")
     @patch("samcli.commands.package.command.do_cli")
     def test_package(
         self,
         do_cli_mock,
         get_template_artifacts_format_mock,
-        cli_validation_artifacts_format_mock,
         is_all_image_funcs_provided_mock,
     ):
         is_all_image_funcs_provided_mock.return_value = True
-        cli_validation_artifacts_format_mock.return_value = [ZIP]
         get_template_artifacts_format_mock.return_value = [ZIP]
         config_values = {
             "template_file": "mytemplate.yaml",
@@ -611,14 +608,12 @@ def test_package_with_image_repository_and_image_repositories(
 
             self.assertIsNotNone(result.exception)
 
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     @patch("samcli.commands._utils.template.get_template_artifacts_format")
     @patch("samcli.commands._utils.options.get_template_artifacts_format")
     @patch("samcli.commands.deploy.command.do_cli")
-    def test_deploy(self, do_cli_mock, template_artifacts_mock1, template_artifacts_mock2, template_artifacts_mock3):
+    def test_deploy(self, do_cli_mock, template_artifacts_mock1, template_artifacts_mock2):
         template_artifacts_mock1.return_value = [ZIP]
         template_artifacts_mock2.return_value = [ZIP]
-        template_artifacts_mock3.return_value = [ZIP]
         config_values = {
             "template_file": "mytemplate.yaml",
             "stack_name": "mystack",
@@ -722,16 +717,14 @@ def test_deploy_image_repositories_and_image_repository(self, do_cli_mock):
             result = runner.invoke(cli, [])
             self.assertIsNotNone(result.exception)
 
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     @patch("samcli.commands._utils.options.get_template_artifacts_format")
     @patch("samcli.commands._utils.template.get_template_artifacts_format")
     @patch("samcli.commands.deploy.command.do_cli")
     def test_deploy_different_parameter_override_format(
-        self, do_cli_mock, template_artifacts_mock1, template_artifacts_mock2, template_artifacts_mock3
+        self, do_cli_mock, template_artifacts_mock1, template_artifacts_mock2
     ):
         template_artifacts_mock1.return_value = [ZIP]
         template_artifacts_mock2.return_value = [ZIP]
-        template_artifacts_mock3.return_value = [ZIP]
 
         config_values = {
             "template_file": "mytemplate.yaml",
@@ -928,7 +921,6 @@ def test_info_must_not_read_from_config(self, deps_info_mock, system_info_mock):
 
     @patch("samcli.commands._utils.experimental.is_experimental_enabled")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     @patch("samcli.commands._utils.template.get_template_artifacts_format")
     @patch("samcli.commands._utils.options.get_template_artifacts_format")
     @patch("samcli.commands.sync.command.do_cli")
@@ -937,13 +929,11 @@ def test_sync(
         do_cli_mock,
         template_artifacts_mock1,
         template_artifacts_mock2,
-        template_artifacts_mock3,
         is_all_image_funcs_provided_mock,
         experimental_mock,
     ):
         template_artifacts_mock1.return_value = [ZIP]
         template_artifacts_mock2.return_value = [ZIP]
-        template_artifacts_mock3.return_value = [ZIP]
         is_all_image_funcs_provided_mock.return_value = True
         experimental_mock.return_value = True
 

diff --git a/tests/unit/lib/cli_validation/test_image_repository_validation.py b/tests/unit/lib/cli_validation/test_image_repository_validation.py
@@ -33,11 +33,10 @@ def foo():
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
-    def test_image_repository_validation_success_ZIP(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+    def test_image_repository_validation_success_no_image_repository_required(
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
-        mock_artifacts.return_value = [ZIP]
+
         is_all_image_funcs_provided_mock.return_value = True
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [False, False, False, False, False, None, MagicMock()]
@@ -47,12 +46,11 @@ def test_image_repository_validation_success_ZIP(
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     def test_image_repository_validation_success_IMAGE_image_repository(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
-        mock_artifacts.return_value = [IMAGE]
-        is_all_image_funcs_provided_mock.return_value = True
+
+        is_all_image_funcs_provided_mock.return_value = False
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [
             False,
@@ -69,11 +67,10 @@ def test_image_repository_validation_success_IMAGE_image_repository(
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     def test_image_repository_validation_success_IMAGE_image_repositories(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
-        mock_artifacts.return_value = [IMAGE]
+
         is_all_image_funcs_provided_mock.return_value = True
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [
@@ -90,12 +87,31 @@ def test_image_repository_validation_success_IMAGE_image_repositories(
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
+    def test_image_repository_validation_success_IMAGE_image_repository_with_no_all_image_func_provided(
+        self, is_all_image_funcs_provided_mock, mock_click
+    ):
+
+        is_all_image_funcs_provided_mock.return_value = False
+        mock_context = MagicMock()
+        mock_context.params.get.side_effect = [
+            False,
+            False,
+            "123456789012.dkr.ecr.us-east-1.amazonaws.com/test1",
+            False,
+            False,
+            None,
+            MagicMock(),
+        ]
+        mock_click.get_current_context.return_value = mock_context
+        self.foobar()
+
+    @patch("samcli.lib.cli_validation.image_repository_validation.click")
+    @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
     def test_image_repository_validation_failure_IMAGE_image_repositories_and_image_repository(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
         mock_click.BadOptionUsage = click.BadOptionUsage
-        mock_artifacts.return_value = [IMAGE]
+
         is_all_image_funcs_provided_mock.return_value = True
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [
@@ -124,12 +140,11 @@ def test_image_repository_validation_failure_IMAGE_image_repositories_and_image_
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     def test_image_repository_validation_failure_IMAGE_image_repositories_incomplete(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
         mock_click.BadOptionUsage = click.BadOptionUsage
-        mock_artifacts.return_value = [IMAGE]
+
         is_all_image_funcs_provided_mock.return_value = False
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [
@@ -149,12 +164,36 @@ def test_image_repository_validation_failure_IMAGE_image_repositories_incomplete
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
+    def test_image_repository_validation_failure_IMAGE_no_image_repository_with_no_all_image_func_provided(
+        self, is_all_image_funcs_provided_mock, mock_click
+    ):
+        mock_click.BadOptionUsage = click.BadOptionUsage
+
+        is_all_image_funcs_provided_mock.return_value = False
+        mock_context = MagicMock()
+        mock_context.params.get.side_effect = [False, False, False, None, False, None, MagicMock()]
+        mock_click.get_current_context.return_value = mock_context
+
+        with self.assertRaises(click.BadOptionUsage) as ex:
+            self.foobar()
+        if self.support_resolve_image_repos:
+            self.assertIn(
+                "Missing option '--image-repositories', '--image-repository', '--resolve-image-repos'",
+                ex.exception.message,
+            )
+        else:
+            self.assertIn(
+                "Missing option '--image-repositories', '--image-repository'",
+                ex.exception.message,
+            )
+
+    @patch("samcli.lib.cli_validation.image_repository_validation.click")
+    @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
     def test_image_repository_validation_failure_IMAGE_missing_image_repositories(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
         mock_click.BadOptionUsage = click.BadOptionUsage
-        mock_artifacts.return_value = [IMAGE]
+
         is_all_image_funcs_provided_mock.return_value = False
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [False, False, False, None, False, None, MagicMock()]
@@ -175,13 +214,12 @@ def test_image_repository_validation_failure_IMAGE_missing_image_repositories(
 
     @patch("samcli.lib.cli_validation.image_repository_validation.click")
     @patch("samcli.lib.cli_validation.image_repository_validation._is_all_image_funcs_provided")
-    @patch("samcli.lib.cli_validation.image_repository_validation.get_template_artifacts_format")
     def test_image_repository_validation_success_missing_image_repositories_guided(
-        self, mock_artifacts, is_all_image_funcs_provided_mock, mock_click
+        self, is_all_image_funcs_provided_mock, mock_click
     ):
         # Guided allows for filling of the image repository values.
         mock_click.BadOptionUsage = click.BadOptionUsage
-        mock_artifacts.return_value = [IMAGE]
+
         is_all_image_funcs_provided_mock.return_value = False
         mock_context = MagicMock()
         mock_context.params.get.side_effect = [True, True, False, None, False, None, MagicMock()]