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

Added partion to client meta object #1027

Merged
merged 1 commit into from Oct 7, 2016
Merged

Added partion to client meta object #1027

merged 1 commit into from Oct 7, 2016

Conversation

Posnet
Copy link
Contributor

@Posnet Posnet commented Sep 9, 2016

I found that I needed access to the partition being used for a particular client, so that I can write tools that work in both normal AWS regions as well as GovCloud and China.
I view this as a feature similar to the region_name property of the client meta object.

I have added the partition as a property on the client meta object.
I have added unit tests for new meta property.

The one part I am not sure about to what to do if no partition is set for an endpoint_config.
For the moment I have set it to be None, which seems like a reasonable fallback, though I am happy for advice on a better default value. Possibly just setting 'aws' as the fallback partition.

@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Current coverage is 97.60% (diff: 100%)

Merging #1027 into develop will increase coverage by <.01%

@@            develop      #1027   diff @@
==========================================
  Files            44         44          
  Lines          6894       6898     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6729       6733     +4   
  Misses          165        165          
  Partials          0          0          

Powered by Codecov. Last update 34fa1ea...0296ded

@jamesls
Copy link
Member

jamesls commented Sep 9, 2016

Can you give me a little background info on how you'd use the partition information for a given client? I'd just like to understand the use case a little better.

@Posnet
Copy link
Contributor Author

Posnet commented Sep 11, 2016

This is specifically for a stage in our tooling where we enhance cloudtrails information with data we manually pull from the API.

At the moment we have a hard coded mapping from regions to partition so that our scraping tool (similar to netflix edda) is able to populate the cloudtrails record with more data before forwarding it to our log aggregation platform.

This works fine for us, but it seemed like a cleaner solution to have it available from the client object. Our current mapping code actually uses the botocore data files to lookup the region names and work out the partition, very similar to my submitted pull request.

@jamesls
Copy link
Member

jamesls commented Sep 13, 2016

Seems reasonable to me. Our ClientMeta has region_name and endpoint_url. Exposing partition seems fine. We'll need to move the tests over to tests/functional. I think that's why codecov is complaining. We don't run integration tests during our travis builds.

@kyleknap @JordonPhillips any concerns?

@kyleknap
Copy link
Member

@jamesls I am fine with this and yes let's get these moved over from integration tests to unit or functional.

@Posnet
Copy link
Contributor Author

Posnet commented Sep 19, 2016

I have moved the tests to the unit tests

@jamesls
Copy link
Member

jamesls commented Sep 19, 2016

Looks good to me. I think I'd still like to also see a functional test that uses a real client via session.create_client(...) to ensure everything's hooked up properly.

@Posnet
Copy link
Contributor Author

Posnet commented Sep 20, 2016

I have added functional tests as requested.

@Posnet
Copy link
Contributor Author

Posnet commented Sep 26, 2016

Any update on whether this can be merged?

@Posnet
Copy link
Contributor Author

Posnet commented Oct 3, 2016

Is there anything else I need to do for this pull request?

@jamesls
Copy link
Member

jamesls commented Oct 6, 2016

Could you remove the tests/coverage.xml file? Looks like it was added by accident.

Otherwise, the test updates look good to me, let's just get one more reviewer to look at it.

cc @kyleknap @JordonPhillips

Made the partion name available in the client meta object
Added unit tests for partition in client metadata
Added functional tests for partition in client metadata
@Posnet
Copy link
Contributor Author

Posnet commented Oct 6, 2016

I have now removed tests/coverage.xml, sorry about that.

@jamesls jamesls added the pr/needs-review This PR needs a review from a member of the team. label Oct 7, 2016
Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. @jamesls should be good to merge

@jamesls jamesls merged commit 0296ded into boto:develop Oct 7, 2016
@jamesls
Copy link
Member

jamesls commented Oct 7, 2016

Merged, thanks for the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants