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

Resolves #93: Uses boto3 to find region if possible #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

artburkart
Copy link
Contributor

Rather than depending on the partially implemented AWS_DEFAULT_REGION
check, this change makes it so credstash relies more heavily on boto3 to
discover regions. Now we can do things like define an ~/.aws/config
file that looks like this:

[profile work]
region=us-west-2

and run credstash like this:

AWS_PROFILE=work credstash -n arn:aws:iam::000000000000:role/some-role get test

and it will successfully find the correct region

Only in the case where the region is absolutely not found, does
credstash then default to us-east-1. It's slightly backward
incompatible, but I think it's a nice improvement.

@artburkart artburkart changed the title Resolves #93: Uses boto3 to fid region if possible Resolves #93: Uses boto3 to find region if possible Oct 13, 2017
Rather than depending on the partially implemented `AWS_DEFAULT_REGION`
check, this change makes it so credstash relies more heavily on boto3 to
discover regions. Now we can do things like define an `~/.aws/config`
file that looks like this:

```ini
[profile work]
region=us-west-2
```

and run credstash like this:

```command
AWS_PROFILE=work credstash -n arn:aws:iam::000000000000:role/some-role get test
```

and it will successfully find the correct region

Only in the case where the region is absolutely not found, does
credstash then default to us-east-1. It's slightly backward
incompatible, but I think it's a nice improvement.
@artburkart artburkart force-pushed the i-93-use-boto3-discover-region branch from d3efcb9 to 5275a06 Compare October 13, 2017 02:58
@wayne-luminal
Copy link
Contributor

wayne-luminal commented Oct 26, 2017

@artburkart Thanks for another PR!

What do you think about dropping a default region from credstash? So I'm thinking the resolution rules would be:

  • --region parameter
  • Let boto* go through its own rules
  • If region is not found raise an exception

What do you think about that?

Thank you for pointing out a backward incompatible change. Given the number of PRs that are outstanding, I'm going through all of them now to create a new major version of credstash that will include backward incompatible changes so it's good to call those out when you see them so they can be documented.

@artburkart
Copy link
Contributor Author

While I don't think it will impact me personally, it could break backward compatibility for some (maybe many?) of the current users out there. Would there be a way to tell users this is a breaking change?

@wayne-luminal
Copy link
Contributor

What I'd like to do is start following semver conventions for releases. Major version bumps indicate breaking changes or major updates to functionality. Breaking changes would also be documented, probably in the release notes. We could also do beta releases that are usable for some time before committing to a full on release to give folks time to adjust, though that would be a little more overhead.

@artburkart
Copy link
Contributor Author

artburkart commented Oct 27, 2017 via email

@rubendura
Copy link

I’m thinking more and more that removing the default and raising an exception in the worst case is the best option. If there is no default way to get a region let the user know and configure it the way they want, either using boto3 default chain of config or the —region shortcut.

@artburkart
Copy link
Contributor Author

How soon do you need this? I don't want to hold anyone up if you wanted it yesterday or something like that :)

@rubendura
Copy link

Well, I'm no longer using CredStash in any of my projects for various reasons (including that I changed employer), so I don't need it any time soon :P Still, I think this change makes for a better experience overall.

@artburkart
Copy link
Contributor Author

Heh, I figured it wasn't too urgent for you, since the request was made over a year ago. I was more curious whether there was any urgency on the luminal side of things. :) Regardless, I made the update.

@inhumantsar
Copy link

@artburkart @wayne-luminal i'd really like to see this PR implemented. should it be redone from the ground up or is there something we can do to nudge it over the line?

@artburkart
Copy link
Contributor Author

@inhumantsar – I think the PR is still good as-is, but I'd need to resolve one minor plaintext conflict and get it approved and merged for it to go live.

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.

4 participants