-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Extract the logic of managing SAM cloudformation stack from the the S… #2740
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
Conversation
…3-bucket bootstrap logic
| if profile: | ||
| session = boto3.Session(profile_name=profile, region_name=region if region else None) | ||
| cloudformation_client = session.client("cloudformation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a new logic but doesn't not result in any behavior change as the method caller(see bootstrap.py) always pass profile=None (which how it used to be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not create a new session, the session is handled here
aws-sam-cli/samcli/cli/context.py
Lines 169 to 186 in f084cf2
| def _refresh_session(self): | |
| """ | |
| Update boto3's default session by creating a new session based on values set in the context. Some properties of | |
| the Boto3's session object are read-only. Therefore when Click parses new AWS session related properties (like | |
| region & profile), it will call this method to create a new session with latest values for these properties. | |
| """ | |
| try: | |
| botocore_session = botocore.session.get_session() | |
| boto3.setup_default_session( | |
| botocore_session=botocore_session, region_name=self._aws_region, profile_name=self._aws_profile | |
| ) | |
| # get botocore session and setup caching for MFA based credentials | |
| botocore_session.get_component("credential_provider").get_provider( | |
| "assume-role" | |
| ).cache = credentials.JSONFileCache() | |
| except botocore.exceptions.ProfileNotFound as ex: | |
| raise CredentialsError(str(ex)) from ex |
so whenever we need to apply any changes to the AWS services session we can do it from this location only, like if we need to use specific services endpoints, so we can change in the default session, and we are sure that it will be applied to all AWS services connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use case is different. The one you are referring to is for the --profile option where we execute a SAM CLI command against one AWS account. But for this use case we want to deploy a CFN template to multiple AWS accounts so we don't manage the AWS account through this --profile option.
| ) | ||
| raise UserException(msg) | ||
| outputs = manage_cloudformation_stack( | ||
| profile=None, region=region, stack_name=SAM_CLI_STACK_NAME, template_body=_get_stack_template() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting profile to None as the existing code didn't use this param anywhere
| except ProfileNotFound as ex: | ||
| raise CredentialsError( | ||
| f"Error Setting Up Managed Stack Client: the provided AWS name profile '{profile}' is not found. " | ||
| "please check the documentation for setting up a named profile: " | ||
| "https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-profiles.html" | ||
| ) from ex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a new logic but doesn't not result in any behavior change as the method caller(see bootstrap.py) always pass profile=None (which how it used to be) so this ProfileNotFoundexception will never be raised
Extract the logic of managing SAM CloudFormation stack to a utility module
Which issue(s) does this change fix?
NA
Why is this change necessary?
SAM bootstrap creates/manages a S3 bucket via a CFN stack("aws-sam-cli-managed-default"). This change is to decoupl the logic of managing a SAM CFN stack from this particular "aws-sam-cli-managed-default" stack so that the logic can be reused to manage any other SAM CFN stacks.
How does it address the issue?
What side effects does this change have?
Nothing, this refactor doesn't cause any behavior change.
Checklist
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.