-
Notifications
You must be signed in to change notification settings - Fork 166
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
Change default region in ssmstore lookup. #595
base: master
Are you sure you want to change the base?
Change default region in ssmstore lookup. #595
Conversation
Signed-off-by: Kai Xia <[email protected]>
Sorry reviewers, I would love to add a test for this change, but I cannot think of a good way to do it. If you've got some ideas, I'd love to implement them. CircleCI failed for some obscure reason that I cannot understand, I guess its not related to the change I've made? |
Hey @xiaket - thanks for this PR! A couple of things:
If you update it from the current master in remind101/stacker, and then push your changes that should clear this up.
Let me know if you have any issues digging into either of these, and thanks again for the PR! |
add another set of client/stubber/response for the region change. Signed-off-by: Kai Xia <[email protected]>
Thanks @phobologic ! I had tried to make a little contribution back then and I totally forget about it until you pointed out, that's a stupid mistake. Anyway, I've also added the tests, hope that will make sense. |
Hey @xiaket - sorry for not getting back to you sooner. This PR actually made me realize that we potentially have a bit bigger of an issue - mostly that our lookups don't use any common way of determining what region to use. I think that might be a good thing to do going forward - and would be more easily testable in general (really, the logic isn't around if we use the right end point provided with a region - the logic is how our lookups CHOOSE their region). I'm wondering if we should instead have a common helper method for determining the region used by lookups? Going to ping some other folks to get their opinion here - @troyready @ejholmes @danielkza @GarisonLotus @russellballestrini @Lowercases |
Thanks, @phobologic for your reply! I agree, adding a new general way to choose the aws region would be really useful. |
I'd prefer this to go into a 2.0 branch, since it technically changes existing behavior (there may be users deploying to oregon/etc that have parameters in virginia and are relying on the default lookup). Agree on the idea of adding a common method as well. This implementation seems fine for now - not sure how it interacts with the new multi-profile/region support? |
Codecov Report
@@ Coverage Diff @@
## master #595 +/- ##
========================================
Coverage ? 89.4%
========================================
Files ? 92
Lines ? 5730
Branches ? 0
========================================
Hits ? 5123
Misses ? 607
Partials ? 0
Continue to review full report at Codecov.
|
In most cases, user have specified the AWS region from the command line, and we can just get this region from the provider and retrieve parameters from that region. If this lookup failed, still fall back to
us-east-1
.