aws-sam-cli icon indicating copy to clipboard operation
aws-sam-cli copied to clipboard

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

Open defenderkev opened this issue 2 years ago • 5 comments

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?)
  • [x] Write/update unit tests
  • [n/a] Write/update integration tests
  • [n/a] Write/update functional tests if needed
  • [x] make pr passes
  • [n/a] make update-reproducible-reqs if dependencies were changed
  • [x] 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.

defenderkev avatar Jan 17 '24 17:01 defenderkev

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 avatar Jan 23 '24 01:01 hnnasit

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

kevin-james-sp avatar Jan 25 '24 10:01 kevin-james-sp

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

hnnasit avatar Jan 25 '24 23:01 hnnasit

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

defenderkev avatar Jan 31 '24 21:01 defenderkev

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 avatar Feb 05 '24 16:02 hnnasit

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

hnnasit avatar Feb 29 '24 06:02 hnnasit

Sorry, work has been crazy recently. I'm still willing to continue with this, but I just haven't had time to review your suggestions yet. I hope to get some time in the next couple of weeks

On Thu, Feb 29, 2024 at 6:25 AM Haresh Nasit @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/aws/aws-sam-cli/pull/6568#issuecomment-1970488111, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAMIXGDB7YO4RTGLUQAZGCDYWANMJAVCNFSM6AAAAABB66I3KOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZQGQ4DQMJRGE . You are receiving this because you were mentioned.Message ID: @.***>

defenderkev avatar Mar 01 '24 13:03 defenderkev

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 avatar Mar 25 '24 21:03 mildaniel

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!

hawflau avatar Apr 01 '24 17:04 hawflau