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

Find the partition of a region #1715

Merged
merged 3 commits into from
Oct 27, 2021
Merged

Find the partition of a region #1715

merged 3 commits into from
Oct 27, 2021

Conversation

jkinred
Copy link
Contributor

@jkinred jkinred commented Apr 13, 2019

This commit implements the core functionality for a feature request
against boto3.

See boto/boto3#1868.

I implemented the method with a more verbose name. Happy to adjust if needed.

@codecov-io
Copy link

codecov-io commented Apr 13, 2019

Codecov Report

Merging #1715 into develop will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1715      +/-   ##
===========================================
- Coverage    92.42%   92.41%   -0.02%     
===========================================
  Files           53       53              
  Lines         9872     9884      +12     
===========================================
+ Hits          9124     9134      +10     
- Misses         748      750       +2
Impacted Files Coverage Δ
botocore/session.py 98.09% <100%> (+0.01%) ⬆️
botocore/regions.py 96.55% <77.77%> (-2.17%) ⬇️

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 d8f3892...b19476e. Read the comment docs.

@shorea
Copy link

shorea commented Oct 3, 2019

@joguSD is this something that can be merged? This would be a pretty useful feature to have.

@jkinred
Copy link
Contributor Author

jkinred commented Oct 14, 2019

If this is a useful feature that may be merged, I will invest some time in cleaning up the checks and rebasing.

@jeking3
Copy link

jeking3 commented Feb 21, 2020

I would make use of this if it was merged as well. It seems pretty useful for ARN generation.

@aph3rson
Copy link

+1ing this PR as well, this would be helpful for instances where the partition is necessary for certain functionality (e.g. determining console and signin URLs, which my tool depends on).

@jonrau1
Copy link

jonrau1 commented Apr 24, 2020

Throwing another +1 into the mix. My tool, ElectricEye, relies on the partition a lot for creation resource ARNs and creating Security Hub findings. This would allow me to not have to hard-code partitions and create separate implementations per partition.

@jfuss
Copy link

jfuss commented Jun 11, 2020

This would be really useful for SAM as well. Is there a way we can get this merged @kyleknap or @stealthycoin?

@jfuss
Copy link

jfuss commented Jun 12, 2020

@nateprewitt or @joguSD might be better people to ask merge-ablity of the feature.

@wong-a
Copy link

wong-a commented May 7, 2021

Another +1! This is a useful method. What's blocking it?

@yuan-bwn
Copy link

+1 for visibility!
It would be really great customer can fetch this info directly from boto instead of implementing all different sorts of helper methods.

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Hey @jkinred, I think we've got the capacity to try to get this into an upcoming release. If you have a moment to rebase this onto the current develop branch and address the feedback, that would be very helpful. Otherwise, I'll look at building on this PR to get it ready for merging.

tests/unit/test_session.py Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
This commit implements the core functionality for a feature request
against `boto3`.

See boto/boto3#1868.
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #1715 (2c4c368) into develop (26528a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1715   +/-   ##
========================================
  Coverage    95.32%   95.33%           
========================================
  Files           59       59           
  Lines        11474    11484   +10     
========================================
+ Hits         10938    10948   +10     
  Misses         536      536           
Impacted Files Coverage Δ
botocore/exceptions.py 99.51% <100.00%> (+<0.01%) ⬆️
botocore/regions.py 93.81% <100.00%> (+0.33%) ⬆️
botocore/session.py 96.62% <100.00%> (+0.02%) ⬆️

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 26528a5...2c4c368. Read the comment docs.

@nateprewitt
Copy link
Contributor

Thanks @jkinred, I resolved the merge issues from the last rebase, updated the error name, and added some additional tests. I think we're ready for review now, I've tagged a couple more devs to give final sign off since I've added some changes.

Copy link
Contributor

@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. Just had a couple of comments.

@@ -142,6 +144,19 @@ def construct_endpoint(self, service_name, region_name=None, partition_name=None
if result:
return result

def get_partition_for_region(self, region_name):
for partition in self._endpoint_data['partitions']:
if region_name in partition['regions']:
Copy link
Contributor

@kyleknap kyleknap Oct 26, 2021

Choose a reason for hiding this comment

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

We should probably see if we can reuse the _region_match method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, let me get that updated.

botocore/regions.py Show resolved Hide resolved
Copy link
Contributor

@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! 🚢

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.