Skip to content
This repository has been archived by the owner on Jan 20, 2020. It is now read-only.

Arbitrary names for the default profile #21

Merged
merged 9 commits into from
Apr 12, 2018
Merged

Arbitrary names for the default profile #21

merged 9 commits into from
Apr 12, 2018

Conversation

brainstorm
Copy link
Contributor

Hello Coinbase,

Here's a simple changeset that does not break backwards compatibility (default profile stays the same if env var is not defined).

This change just gives some flexibility for the assumption of the default profile naming.

Please let us know if you require further changes, we would like to have those changes upstream :)

/ping @reisingerf

@heimdall-asguard
Copy link

Reviews 0/1 Message

More info

@grahamjenson
Copy link
Contributor

Could you please make it so that the user can override the ROLE_SESSION_TIMEOUT variable. and default to 1 hour.

@grahamjenson
Copy link
Contributor

This is awesome, just tested locally and it works well.

Just two things left:

  1. I think AWS_DEFAULT_PROFILE_ASSUME_ROLE should be called AWS_PROFILE_ASSUME_ROLE because without the envar it is default.
  2. Can you get the tests passing. I left some comments on how to do this.

Cheers

assume-role Outdated
@@ -252,14 +272,17 @@ assume-role(){

# USED FOR TESTING AND DEBUGGING
if [ "$DEBUG_ASSUME_ROLE" = "true" ]; then
echo "AWS_DEFAULT_PROFILE_ASSUME_ROLE=\"$AWS_DEFAULT_PROFILE_ASSUME_ROLE\";"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this output to the end, as the majority of tests take the lines to ensure their order.

assume-role Outdated
@@ -235,6 +254,7 @@ assume-role(){

# OUTPUTS ALL THE EXPORTS for eval $(assume-role [args])
if [ "$OUTPUT_TO_EVAL" = "true" ]; then
echo "export AWS_DEFAULT_PROFILE_ASSUME_ROLE=\"$AWS_DEFAULT_PROFILE_ASSUME_ROLE\";"
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to the end. Not sure if it is necessary.

@heimdall-asguard
Copy link

Reviews 1/1 Message
grahamjenson 1 approved

More info

@grahamjenson grahamjenson merged commit a8f29c9 into coinbase:master Apr 12, 2018
@heimdall-asguard
Copy link

Reviews 1/1 Message
grahamjenson 1 approved

Pull request is reviewed and can be merged to master!

More info

@grahamjenson
Copy link
Contributor

Cheers I will publish to brew soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants