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 <[email protected]>

* add telemetry

* fix format

---------

Co-authored-by: Mehmet Nuri Deveci <[email protected]>

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

samcli/commands/delete/delete_context.py

@@ -5,21 +5,21 @@
 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
 from samcli.lib.package.ecr_uploader import ECRUploader
 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)
 

tests/unit/commands/delete/test_delete_context.py

@@ -1,15 +1,17 @@
 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
 from samcli.cli.cli_config_file import TomlProvider
 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)