-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement configured endpoint url resolver #2958
Implement configured endpoint url resolver #2958
Conversation
906b678
to
d597716
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks fine. Just had some comments. It would be nice if we can get the unit tests included as part of this PR and we can add the functional tests in a subsequent PR. Mainly it is going to be easier to review the unit tests while looking over all of the classes/interfaces that we are adding.
botocore/configprovider.py
Outdated
@@ -510,6 +517,116 @@ def set_config_provider(self, logical_name, provider): | |||
self._mapping[logical_name] = provider | |||
|
|||
|
|||
class ConfiguredEndpointProviderChain: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In using the approach of making the logic a provider let's:
- Rename this class to
ConfiguredEndpointProvider
. Ending it inProvider
makes it clear this is a provider class. Also the wordChain
now conflates with theChainProvider
and makes sense to get rid of its usage since it does not subclass fromChainProvider
. - Make sure this class subclasses from the
BaseProvider
class and implements any of the shared methods as (e.g.__deepcopy__
and__repr__
) - Consider documenting that users should not be instantiating this class directly and that it is not supported in case we ever want to refactor the initializer arguments for the class.
- Add unit tests for this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added __deepcopy__
method, but __repr__
is tricky. Other config providers have succinct string-based parameters because they also take a session object, so can initialize other objects internally. The init params for ConfiguredEndpointProvider
are all potentially large dictionaries.
e0ff128
to
3b55e1d
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## configured-endpoint-urls #2958 +/- ##
============================================================
- Coverage 93.32% 93.32% -0.01%
============================================================
Files 64 64
Lines 13597 13662 +65
============================================================
+ Hits 12690 12750 +60
- Misses 907 912 +5
☔ View full report in Codecov by Sentry. |
bbc2d5e
to
610ce31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good so far. I'd like to see some organizational changes and have some questions about the underlying logic in a couple of places, but we're definitely on the right track here.
93df202
to
0aebebf
Compare
any news, guys?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just had feedback around organizations and some of the refactors that were done. I do think we are close though to get this one merged so that we can move on to the functional test PR.
9b82ed3
to
659df5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just had some small comments, but we should be set after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just one last comment that I missed from the last review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚢 Feel free to squash the commits and merge.
Adds a config provider to resolve the endpoint URL provided in an environment variable or shared configuration file. It resolves the endpoint in the following manner: 1. The value provided through the `endpoint_url` parameter provided to the client constructor. 2. The value provided by a service-specific environment variable. 3. The value provided by the global endpoint environment variable (`AWS_ENDPOINT_URL`). 4. The value provided by a service-specific parameter from a services definition section in the shared configuration file. 5. The value provided by the global parameter from a services definition section in the shared configuration file. 6. The value resolved when no configured endpoint URL is provided. The endpoint config provider uses the client name (name used to instantiate a client object) for construction and add to the config value store. This uses multiple lookups to handle service name changes for backwards compatibility.
3a58267
to
ba4264f
Compare
Adds a config provider to resolve the endpoint URL provided in an environment variable or shared configuration file. It resolves the endpoint in the following manner:
endpoint_url
parameter provided to the client constructor.AWS_ENDPOINT_URL
).