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

Use AWS shared credentials config #48

Merged
merged 3 commits into from
Jul 15, 2021

Conversation

lox
Copy link
Contributor

@lox lox commented Jul 13, 2021

This enables shared config checks in the AWS Session SDK for setups that use ~/.aws/credentials. See https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html

Note that 508764a makes a change where by a single session is used and passed around vs session.New() invoked everywhere. It's remotely possible this could break things where creating a new session each time was providing some durability around failures and dropouts unintentionally. I don't think this is likely, as there are internal retry mechanisms in the single session. In fact, I think that it's likely that establishing lots of new sessions could lead to rate-limiting and latency.

lox added 2 commits July 13, 2021 13:56
Currently the code creates a new session.Session for every invocation. This
change pulls out that session as a dependency that gets passed into the various
things that invoke it.
This looks for a credentials file in the aws sdk
Copy link
Contributor

@keithduncan keithduncan left a comment

Choose a reason for hiding this comment

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

I wonder whether we should just have this on all the time for the local main.go as the SDK still preferences the environment variables. We’d still use the environment variables from something like aws-vault even if you have a ~/.aws/config.

Looks good either way to me!

@keithduncan
Copy link
Contributor

Realised after approving that you can’t hit the merge button here, let me know what you think about making this always-on vs a flag then I can merge or we’ll tweak it and merge 🙇‍♂️

@lox
Copy link
Contributor Author

lox commented Jul 15, 2021

Yeah, after some thought just as a default makes sense.

@lox
Copy link
Contributor Author

lox commented Jul 15, 2021

I'd suggest merge and squash for this one to remove the confusing history!

@lox lox requested a review from keithduncan July 15, 2021 00:05
@keithduncan keithduncan merged commit a4c8be3 into buildkite:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants