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

perform exhaustive region discovery #9

Merged
merged 2 commits into from
Jun 11, 2020

Conversation

bwagner5
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:

  • Perform a more exhaustive region discovery
    • Take --region flag if set over all other methods
    • If profile is specified check the profile config for region
    • Check AWS_REGION env var (implicitly checked via SDK on session creation)
    • Check default profile config for region
    • Check for DEFAULT_REGION env var

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bwagner5 bwagner5 requested a review from haugenj June 11, 2020 16:46
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2020

Codecov Report

Merging #9 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #9   +/-   ##
=======================================
  Coverage   89.34%   89.34%           
=======================================
  Files           7        7           
  Lines         779      779           
=======================================
  Hits          696      696           
  Misses         54       54           
  Partials       29       29           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fea12d...5a52c9e. Read the comment docs.

cmd/main.go Outdated
Comment on lines 285 to 292
if defaultProfileRegion, err := getProfileRegion(defaultProfile); err == nil {
sess.Config.Region = &defaultProfileRegion
}
if sess.Config.Region == nil || *sess.Config.Region == "" {
if defaultRegion, ok := os.LookupEnv(defaultRegionEnvVar); ok && defaultRegion != "" {
sess.Config.Region = &defaultRegion
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some logs in here when the resolution fails? Similar to line 273

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to add warning in the other places because they're not required to be specified, so I didn't want a warning to come up all the time when someone only has their AWS_REGION env var set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add a new pretty long error message when no region could be found that specifies the places it looked.

cmd/main.go Outdated

func getProfileRegion(profileName string) (string, error) {
if profileName != defaultProfile {
profileName = fmt.Sprintf("%s %s", "profile", profileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this better than writing fmt.Sprintf("profile %s", profileName) ? Also, why not print when it's the default profile?

Copy link
Contributor Author

@bwagner5 bwagner5 Jun 11, 2020

Choose a reason for hiding this comment

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

haha nope, fmt.Sprintf("profile %s", profileName) is way clearer :)

But it's not printing, it's creating a string with the static string "profile" prepended. The AWS config file prepends profile for custom profiles but does not for the "default" profile.

Here's an example file:

$ cat ~/.aws/config
[default]
region = us-east-1
output = json
[profile jason]
region = us-east-2
output = json
[profile test]
output = json

Copy link
Contributor

@haugenj haugenj left a comment

Choose a reason for hiding this comment

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

👍👍

@bwagner5 bwagner5 merged commit ec04254 into aws:master Jun 11, 2020
@bwagner5 bwagner5 deleted the region-discovery branch July 15, 2020 23:08
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.

None yet

3 participants