Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor init_clients in sam delete #5360

Merged
merged 8 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 21 additions & 30 deletions samcli/commands/delete/delete_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -108,38 +108,29 @@ 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If customer is running this command with no_prompts then we are setting region to us-east-1 by default, is it OK to remove it with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is why should we default to us-east-1? That doesn't sound reasonable to me. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree this is different than other commands, my vote would be keeping the commands in the same way but since we released sam delete with this default, I am not sure if we need to make an announcement or need to keep the backwards compatibility.

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()

# 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
client_provider = get_boto_client_provider_with_config(
region=self.region if self.region else None, profile=self.profile if self.profile else None
hawflau marked this conversation as resolved.
Show resolved Hide resolved
)

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)

Expand Down
48 changes: 47 additions & 1 deletion tests/unit/commands/delete/test_delete_context.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -518,3 +520,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)