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

Change default region in ssmstore lookup. #595

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

Change default region in ssmstore lookup. #595

wants to merge 3 commits into from

Conversation

xiaket
Copy link
Contributor

@xiaket xiaket commented May 11, 2018

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.

@xiaket
Copy link
Contributor Author

xiaket commented May 11, 2018

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?

@phobologic
Copy link
Member

Hey @xiaket - thanks for this PR! A couple of things:

  1. The reason you are getting the error in tests is because your stacker fork master branch is very old - it's from back in october of 2017:

screenshot 2018-05-12 21 14 48

If you update it from the current master in remind101/stacker, and then push your changes that should clear this up.

  1. There are a couple of ways to write tests for this - first, it might be good to create a generic function for getting the region for lookups that matches what this does, then you can test it with various permutations. Second, if you want to test actual queries against the AWS API, you can do so with the botocore.Stubber class. You can find docs here: http://botocore.readthedocs.io/en/latest/reference/stubber.html and plenty of examples of it's use in https://github.com/remind101/stacker/tree/f8f83c3ab50cd454e8aff2990b94b28b52fd5f04/stacker/tests

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]>
@xiaket
Copy link
Contributor Author

xiaket commented May 15, 2018

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.

@phobologic
Copy link
Member

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

@xiaket
Copy link
Contributor Author

xiaket commented May 26, 2018

Thanks, @phobologic for your reply! I agree, adding a new general way to choose the aws region would be really useful.

@troyready
Copy link
Contributor

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-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@a741a90). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #595   +/-   ##
========================================
  Coverage          ?   89.4%           
========================================
  Files             ?      92           
  Lines             ?    5730           
  Branches          ?       0           
========================================
  Hits              ?    5123           
  Misses            ?     607           
  Partials          ?       0
Impacted Files Coverage Δ
stacker/lookups/handlers/ssmstore.py 100% <100%> (ø)

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 a741a90...3a3aaab. Read the comment docs.

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