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

Added the ability to specify --accountid parameter on the command line #6568

Closed
wants to merge 1 commit into from

Conversation

defenderkev
Copy link
Contributor

Which issue(s) does this change fix?

#2325

Why is this change necessary?

Because currently if you have !Sub xxx${AWS::AccountId}xxx in, for example, a layer definition, SAM uses a default substitution which doesn't reference the correct Account ID

How does it address the issue?

It adds the ability to specify a command line option( --accountid ). This parameter is then used during the substitution of parameters in the CloudFormation template to replace ${AWS::AccountId} rather than using a hardcoded default (123456789012)

What side effects does this change have?

None that I can see

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • [n/a] Add input/output type hints to new functions/methods
  • [n/a ] Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • [n/a] Write/update integration tests
  • [n/a] Write/update functional tests if needed
  • make pr passes
  • [n/a] make update-reproducible-reqs if dependencies were changed
  • Write documentation
    - parameter description added to 'AWS Credential Options' section for --help

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This parameter is then used during the substitution of parameters in the CloudFormation template
to replace ${AWS::AccountId} rather than using a hardcoded default (123456789012)
This is useful, for example, when running `sam local invoke` on a Lambda function, where the account id
is not hardcoded into the CFN template in Layer references
Tests have also been updated to expect the accountid parameter
@defenderkev defenderkev requested a review from a team as a code owner January 17, 2024 17:44
@github-actions github-actions bot added area/package sam package command area/deploy sam deploy command area/validate sam validate command area/logs sam logs command area/build sam build command area/local/invoke sam local invoke command pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Jan 17, 2024
@hnnasit
Copy link
Contributor

hnnasit commented Jan 23, 2024

Hi @defenderkev, thanks for opening a PR. I consulted with the team and we agreed to read the account-id from the credentials file and resolve it rather than adding a new flag to all the commands. Would you be open to update your PR with those changes?

@hnnasit hnnasit removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jan 23, 2024
@kevin-james-sp
Copy link
Contributor

Certainly. Once I commit the update, will that notify you or do I need to bump the PR somehow?

@hnnasit
Copy link
Contributor

hnnasit commented Jan 25, 2024

You can notify us once the PR is ready for review and we can take a look.

@defenderkev
Copy link
Contributor Author

Can you confirm which file you're referring to? The only place I can see where the AWS CLI stores the account id is in the config file when using Identity Center SSO - the standard ~/.aws/credentials file does not store the account id.
The page I referenced here is https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html

@hnnasit
Copy link
Contributor

hnnasit commented Feb 5, 2024

Hi @defenderkev, apologies for the late reply. You are right, account_id is not defined in the config file except when using SSO credentials. I was thinking in the sense that the credentials in that file could be used to create a sts boto3 client to retrieve the account_id. An alternate solution could also be to define the account_id as a parameter and read the provided AWS::AccountId as --parameter-overrides value to parse in SAM CLI as @Bradleywboggs mentions here.

@hnnasit
Copy link
Contributor

hnnasit commented Feb 29, 2024

Hi @defenderkev, just checking in if the above suggestions make sense to you. Are you still willing to continue contributions on this PR?

@defenderkev
Copy link
Contributor Author

defenderkev commented Mar 1, 2024 via email

@mildaniel
Copy link
Contributor

mildaniel commented Mar 25, 2024

I'm going to convert this PR to draft for now since it is still being worked on, but not actively. Let us know if there is anything you need on our end to help push this forward.

@mildaniel mildaniel marked this pull request as draft March 25, 2024 21:39
@hawflau
Copy link
Contributor

hawflau commented Apr 1, 2024

Hey @defenderkev, closing your PR for now since there's no activity. You can still work on the PR to address the comments. And feel free to re-open the PR once it's ready for review. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build sam build command area/deploy sam deploy command area/local/invoke sam local invoke command area/logs sam logs command area/package sam package command area/validate sam validate command pr/external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants