-
Notifications
You must be signed in to change notification settings - Fork 512
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
Update BUILDING.md
and PUBLISHING-AWS.md
to mention need for aws creds
#2334
Conversation
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.
Looks great to me overall. A couple of very minor things pointed out though.
c3800a4
to
0784925
Compare
Fore-pushed to addressed @stmcginnis comments around grammar and formatting |
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.
Thanks, lgtm.
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.
One small nit
0784925
to
cc2689c
Compare
Force-pushed to resolve @zmrow's comment on removing "usually" - great suggestion! 👏🏼 |
Note: several commands work with AWS services, so there's some shared configuration related to AWS accounts and AWS IAM roles. | ||
For example, you can specify a role to assume before any API calls are made, and a role to assume before any API calls in a specific region. | ||
This can be useful if you want to use roles to control access to the accounts that own AMIs, for example. | ||
See the commented [example Infra.toml](tools/pubsys/Infra.toml.example) for details. |
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.
Are you just updating the documentation to mention the possibility, or did something in the way the tools behaved suggest that you needed to configure a role to assume?
This is a relatively niche option, intended for the case where an otherwise unprivileged EC2 instance should be assuming roles in the same account, or a different account. Whether it's useful really depends on the scenario and the threat model.
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.
Good question: this is mostly mentioning the possibility since I ran into it directly in #2327
But this specific chunk I actually found and copy/pasted from PUBLISHING.md
. It seemed like a good chunk to also utilize here in PUBLISHING-AWS.md
. I agree, it is a bit of a niche use case, but I wasn't sure if it was common enough for people working on Bottlerocket to also mention here.
Several commands referenced in the developer docs interact with AWS services; AMI, EKS, etc. In order to successfully publish AMIs via the quickstart, AWS creds must be setup. This patch updates both BUILDING.md and PUBLISHING-AWS.md to reflect the need to have this setup. Signed-off-by: John McBride <jpmmcb@amazon.com>
cc2689c
to
858f333
Compare
Force-pushed to address Ben's comment on wording in |
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.
💸
Issue number:
Closes #2327
Description of changes:
Testing done:
N/a
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.