From 2b7c9d271d41c25fe01327af6961e23cba6e891b Mon Sep 17 00:00:00 2001 From: Alexis Facques Date: Thu, 10 Aug 2023 10:04:50 +0200 Subject: [PATCH] Fix wrongful "--image-repositories" opt validation when all functions are valid ECR URL (aws#2934) --- .../image_repository_validation.py | 32 +++---- .../unit/commands/samconfig/test_samconfig.py | 14 +--- .../test_image_repository_validation.py | 84 ++++++++++++++----- 3 files changed, 74 insertions(+), 56 deletions(-) diff --git a/samcli/lib/cli_validation/image_repository_validation.py b/samcli/lib/cli_validation/image_repository_validation.py index 8d26ff558eb..9522df77a21 100644 --- 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 index b2e0822c78f..15edb032922 100644 --- 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 index 37f00b1f876..e9de6a0a9a6 100644 --- 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()]