From fd09a41358e6e8e45ff4f5d54dbef0a5795fe244 Mon Sep 17 00:00:00 2001 From: Wing Fung Lau <4760060+hawflau@users.noreply.github.com> Date: Wed, 14 Jun 2023 11:27:12 -0700 Subject: [PATCH] refactor init_clients in sam delete (#5360) * refactor init_clients in sam delete * remove unused line * use client_provider * fix broken tests * Update samcli/commands/delete/delete_context.py Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * add telemetry * fix format --------- Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> --- samcli/commands/delete/command.py | 2 + samcli/commands/delete/delete_context.py | 51 ++++---- .../commands/delete/test_delete_context.py | 112 ++++++++++++++---- 3 files changed, 109 insertions(+), 56 deletions(-) diff --git a/samcli/commands/delete/command.py b/samcli/commands/delete/command.py index 960b067cbf..ae28122710 100644 --- a/samcli/commands/delete/command.py +++ b/samcli/commands/delete/command.py @@ -9,6 +9,7 @@ from samcli.cli.main import aws_creds_options, common_options, pass_context, print_cmdline_args from samcli.commands._utils.command_exception_handler import command_exception_handler +from samcli.lib.telemetry.metric import track_command from samcli.lib.utils.version_checker import check_newer_version SHORT_HELP = "Delete an AWS SAM application and the artifacts created by sam deploy." @@ -82,6 +83,7 @@ @aws_creds_options @common_options @pass_context +@track_command @check_newer_version @print_cmdline_args @command_exception_handler diff --git a/samcli/commands/delete/delete_context.py b/samcli/commands/delete/delete_context.py index 3ebcf29669..08d327eceb 100644 --- a/samcli/commands/delete/delete_context.py +++ b/samcli/commands/delete/delete_context.py @@ -5,13 +5,13 @@ import logging from typing import Optional -import boto3 import click +from botocore.exceptions import NoCredentialsError, NoRegionError from click import confirm, prompt from samcli.cli.cli_config_file import TomlProvider -from samcli.cli.context import Context from samcli.commands.delete.exceptions import CfDeleteFailedStatusError +from samcli.commands.exceptions import AWSServiceClientError, RegionError from samcli.lib.bootstrap.companion_stack.companion_stack_builder import CompanionStack from samcli.lib.delete.cfn_utils import CfnUtils from samcli.lib.package.artifact_exporter import Template @@ -19,7 +19,7 @@ from samcli.lib.package.local_files_utils import get_uploaded_s3_object_name from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.uploaders import Uploaders -from samcli.lib.utils.boto_utils import get_boto_config_with_user_agent +from samcli.lib.utils.boto_utils import get_boto_client_provider_with_config CONFIG_COMMAND = "deploy" CONFIG_SECTION = "parameters" @@ -108,38 +108,27 @@ def init_clients(self): """ Initialize all the clients being used by sam delete. """ - if not self.region: - if not self.no_prompts: - session = boto3.Session() - region = session.region_name - self.region = region if region else "us-east-1" - else: - # TODO: as part of the guided and non-guided context separation, we need also to move the options - # validations to a validator similar to samcli/lib/cli_validation/image_repository_validation.py. - raise click.BadOptionUsage( - option_name="--region", - message="Missing option '--region', region is required to run the non guided delete command.", - ) - - if self.profile: - Context.get_current_context().profile = self.profile - if self.region: - Context.get_current_context().region = self.region - - boto_config = get_boto_config_with_user_agent() + client_provider = get_boto_client_provider_with_config(region=self.region, profile=self.profile) - # Define cf_client based on the region as different regions can have same stack-names - cloudformation_client = boto3.client( - "cloudformation", region_name=self.region if self.region else None, config=boto_config - ) - - s3_client = boto3.client("s3", region_name=self.region if self.region else None, config=boto_config) - ecr_client = boto3.client("ecr", region_name=self.region if self.region else None, config=boto_config) + try: + cloudformation_client = client_provider("cloudformation") + s3_client = client_provider("s3") + ecr_client = client_provider("ecr") + except NoCredentialsError as ex: + raise AWSServiceClientError( + "Unable to resolve credentials for the AWS SDK for Python client. " + "Please see their documentation for options to pass in credentials: " + "https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html" + ) from ex + except NoRegionError as ex: + raise RegionError( + "Unable to resolve a region. " + "Please provide a region via the --region, via --profile or by the " + "AWS_DEFAULT_REGION environment variable." + ) from ex self.s3_uploader = S3Uploader(s3_client=s3_client, bucket_name=self.s3_bucket, prefix=self.s3_prefix) - self.ecr_uploader = ECRUploader(docker_client=None, ecr_client=ecr_client, ecr_repo=None, ecr_repo_multi=None) - self.uploaders = Uploaders(self.s3_uploader, self.ecr_uploader) self.cf_utils = CfnUtils(cloudformation_client) diff --git a/tests/unit/commands/delete/test_delete_context.py b/tests/unit/commands/delete/test_delete_context.py index 75559d5416..daa72c187b 100644 --- a/tests/unit/commands/delete/test_delete_context.py +++ b/tests/unit/commands/delete/test_delete_context.py @@ -1,8 +1,9 @@ from samcli.lib.bootstrap.companion_stack.data_types import CompanionStack from unittest import TestCase -from unittest.mock import patch, call, MagicMock +from unittest.mock import patch, call, MagicMock, Mock import click +from botocore.exceptions import NoCredentialsError, NoRegionError from samcli.commands.delete.delete_context import DeleteContext from samcli.lib.package.artifact_exporter import Template @@ -10,6 +11,7 @@ from samcli.lib.delete.cfn_utils import CfnUtils from samcli.lib.package.s3_uploader import S3Uploader from samcli.lib.package.ecr_uploader import ECRUploader +from samcli.commands.exceptions import AWSServiceClientError, RegionError from samcli.commands.delete.exceptions import CfDeleteFailedStatusError @@ -18,9 +20,9 @@ class TestDeleteContext(TestCase): @patch("samcli.commands.delete.delete_context.click.echo") @patch("samcli.commands.delete.delete_context.click.get_current_context") @patch.object(CfnUtils, "has_stack", MagicMock(return_value=(False))) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_delete_context_stack_does_not_exist( - self, patched_boto3, patched_click_get_current_context, patched_click_echo + self, get_boto_client_provider_mock, patched_click_get_current_context, patched_click_echo ): with DeleteContext( stack_name="test", @@ -40,8 +42,8 @@ def test_delete_context_stack_does_not_exist( @patch.object(DeleteContext, "parse_config_file", MagicMock()) @patch.object(DeleteContext, "init_clients", MagicMock()) - @patch("boto3.client") - def test_delete_context_enter(self, patched_boto3): + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_delete_context_enter(self, get_boto_client_provider_mock): with DeleteContext( stack_name="test", region="us-east-1", @@ -71,8 +73,8 @@ def test_delete_context_enter(self, patched_boto3): ), ) @patch("samcli.commands.delete.delete_context.click.get_current_context") - @patch("boto3.client") - def test_delete_context_parse_config_file(self, patched_boto3, patched_click_get_current_context): + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_delete_context_parse_config_file(self, get_boto_client_provider_mock, patched_click_get_current_context): patched_click_get_current_context = MagicMock() with DeleteContext( stack_name=None, @@ -94,9 +96,9 @@ def test_delete_context_parse_config_file(self, patched_boto3, patched_click_get @patch("samcli.commands.delete.delete_context.confirm") @patch("samcli.commands.delete.delete_context.click.get_current_context") @patch.object(CfnUtils, "has_stack", MagicMock(return_value=(False))) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_delete_no_user_input( - self, patched_boto3, patched_click_get_current_context, patched_confirm, patched_prompt + self, get_boto_client_provider_mock, patched_click_get_current_context, patched_confirm, patched_prompt ): patched_click_get_current_context = MagicMock() with DeleteContext( @@ -142,8 +144,8 @@ def test_delete_no_user_input( @patch.object(Template, "get_ecr_repos", MagicMock(return_value=({"logical_id": {"Repository": "test_id"}}))) @patch.object(S3Uploader, "delete_prefix_artifacts", MagicMock()) @patch("samcli.commands.delete.delete_context.click.get_current_context") - @patch("boto3.client") - def test_delete_context_valid_execute_run(self, patched_boto3, patched_click_get_current_context): + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_delete_context_valid_execute_run(self, get_boto_client_provider_mock, patched_click_get_current_context): patched_click_get_current_context = MagicMock() with DeleteContext( stack_name=None, @@ -171,9 +173,9 @@ def test_delete_context_valid_execute_run(self, patched_boto3, patched_click_get @patch.object(CfnUtils, "get_stack_template", MagicMock(return_value=({"TemplateBody": "Hello World"}))) @patch.object(CfnUtils, "delete_stack", MagicMock()) @patch.object(CfnUtils, "wait_for_delete", MagicMock()) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_delete_context_no_s3_bucket( - self, patched_boto3, patched_click_get_current_context, patched_click_secho, patched_click_echo + self, get_boto_client_provider_mock, patched_click_get_current_context, patched_click_secho, patched_click_echo ): with DeleteContext( stack_name="test", @@ -211,9 +213,13 @@ def test_delete_context_no_s3_bucket( @patch.object(CfnUtils, "delete_stack", MagicMock()) @patch.object(CfnUtils, "wait_for_delete", MagicMock()) @patch.object(S3Uploader, "delete_artifact", MagicMock()) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_guided_prompts_s3_bucket_prefix_present_execute_run( - self, patched_boto3, patched_click_get_current_context, patched_confirm, patched_get_cf_template_name + self, + get_boto_client_provider_mock, + patched_click_get_current_context, + patched_confirm, + patched_get_cf_template_name, ): patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( @@ -270,9 +276,13 @@ def test_guided_prompts_s3_bucket_prefix_present_execute_run( @patch.object(CfnUtils, "wait_for_delete", MagicMock()) @patch.object(S3Uploader, "delete_artifact", MagicMock()) @patch.object(ECRUploader, "delete_ecr_repository", MagicMock()) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_guided_prompts_s3_bucket_present_no_prefix_execute_run( - self, patched_boto3, patched_click_get_current_context, patched_confirm, patched_get_cf_template_name + self, + get_boto_client_provider_mock, + patched_click_get_current_context, + patched_confirm, + patched_get_cf_template_name, ): patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( @@ -321,9 +331,13 @@ def test_guided_prompts_s3_bucket_present_no_prefix_execute_run( @patch.object(ECRUploader, "delete_ecr_repository", MagicMock()) @patch.object(Template, "get_ecr_repos", MagicMock(side_effect=({}, {"logical_id": {"Repository": "test_id"}}))) @patch.object(CompanionStack, "stack_name", "Companion-Stack-Name") - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_guided_prompts_ecr_companion_stack_present_execute_run( - self, patched_boto3, patched_click_get_current_context, patched_confirm, patched_get_cf_template_name + self, + get_boto_client_provider_mock, + patched_click_get_current_context, + patched_confirm, + patched_get_cf_template_name, ): patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( @@ -398,9 +412,13 @@ def test_guided_prompts_ecr_companion_stack_present_execute_run( @patch.object(ECRUploader, "delete_ecr_repository", MagicMock()) @patch.object(Template, "get_ecr_repos", MagicMock(return_value=({"logical_id": {"Repository": "test_id"}}))) @patch.object(CompanionStack, "stack_name", "Companion-Stack-Name") - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_no_prompts_input_is_ecr_companion_stack_present_execute_run( - self, patched_boto3, patched_click_get_current_context, patched_click_echo, patched_get_cf_template_name + self, + get_boto_client_provider_mock, + patched_click_get_current_context, + patched_click_echo, + patched_get_cf_template_name, ): CfnUtils.get_stack_template.return_value = { "TemplateBody": {"Metadata": {"CompanionStackname": "Companion-Stack-Name"}} @@ -446,9 +464,9 @@ def test_no_prompts_input_is_ecr_companion_stack_present_execute_run( @patch.object(S3Uploader, "delete_prefix_artifacts", MagicMock()) @patch.object(ECRUploader, "delete_ecr_repository", MagicMock()) @patch.object(Template, "get_ecr_repos", MagicMock(side_effect=({}, {"logical_id": {"Repository": "test_id"}}))) - @patch("boto3.client") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") def test_retain_resources_delete_stack( - self, patched_boto3, patched_click_get_current_context, patched_get_cf_template_name + self, get_boto_client_provider_mock, patched_click_get_current_context, patched_get_cf_template_name ): patched_get_cf_template_name.return_value = "hello.template" with DeleteContext( @@ -504,8 +522,8 @@ def test_s3_option_flag(self): ) @patch.object(DeleteContext, "parse_config_file", MagicMock()) @patch.object(DeleteContext, "init_clients", MagicMock()) - @patch("boto3.client") - def test_s3_option_flag_overrides_config(self, patched_boto3): + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_s3_option_flag_overrides_config(self, get_boto_client_provider_mock): with DeleteContext( stack_name="test", region="us-east-1", @@ -518,3 +536,47 @@ def test_s3_option_flag_overrides_config(self, patched_boto3): ) as delete_context: self.assertEqual(delete_context.s3_bucket, "s3_bucket_override") self.assertEqual(delete_context.s3_prefix, "s3_prefix_override") + + @patch.object(DeleteContext, "parse_config_file", MagicMock()) + @patch("samcli.commands.delete.delete_context.click.get_current_context") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_must_throw_error_if_boto3_cannot_resolve_credentials( + self, get_boto_client_provider_mock, patched_get_current_context + ): + boto_client_mock = Mock(side_effect=NoCredentialsError) + get_boto_client_provider_mock.return_value = boto_client_mock + with self.assertRaises(AWSServiceClientError) as ex: + with DeleteContext( + stack_name="test", + region=None, + config_file=None, + config_env=None, + profile="profile_without_creds", + no_prompts=True, + s3_bucket=None, + s3_prefix=None, + ): + get_boto_client_provider_mock.assert_called_once_with(region=None, profile="profile_without_creds") + self.assertIn("Unable to resolve credentials for the AWS SDK for Python client", ex) + + @patch.object(DeleteContext, "parse_config_file", MagicMock()) + @patch("samcli.commands.delete.delete_context.click.get_current_context") + @patch("samcli.commands.delete.delete_context.get_boto_client_provider_with_config") + def test_must_throw_error_if_boto3_cannot_resolve_region( + self, get_boto_client_provider_mock, patched_get_current_context + ): + boto_client_mock = Mock(side_effect=NoRegionError) + get_boto_client_provider_mock.return_value = boto_client_mock + with self.assertRaises(RegionError) as ex: + with DeleteContext( + stack_name="test", + region=None, + config_file=None, + config_env=None, + profile="profile_without_region", + no_prompts=True, + s3_bucket=None, + s3_prefix=None, + ): + get_boto_client_provider_mock.assert_called_once_with(region=None, profile="profile_without_region") + self.assertIn("Unable to resolve a region", ex)