Endpoint resolution v2: builtin parameter values #2759
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2759 +/- ##
===========================================
+ Coverage 95.13% 95.25% +0.11%
===========================================
Files 61 62 +1
Lines 12538 12872 +334
===========================================
+ Hits 11928 12261 +333
- Misses 610 611 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
tests/unit/test_args.py
Outdated
| should = self.args_create._should_set_global_sts_endpoint( | ||
| region_name='us-west-1', endpoint_url=None, endpoint_config=None | ||
| ) | ||
| self.assertTrue(should) |
There was a problem hiding this comment.
Typically we won't test private methods directly, instead verifying the desired behavior at whatever the top level interface is. That allows us to make more substantial signature changes without having to update existing tests.
How are you intending to retrieve this from the args module? We may need to expose a "public" method even if it's not intended for use, similar to compute_s3_config. We can note support expectations in the docstring.
There was a problem hiding this comment.
How are you intending to retrieve this from the args module?
_should_set_global_sts_endpoint is a method in the class ClientArgsCreator. The assignment of builtin parameter values happens within the same class.
This is, in fact, the reason why the _compute_endpoint_resolver_v2_builtin_defaults ended up in ClientArgsCreator: Most of the required information is available locally in this class.
tests/unit/test_args.py
Outdated
| ) | ||
| self.assertEqual(bins['AWS::S3::DisableMultiRegionAccessPoints'], True) | ||
|
|
||
| def test_builtin_sdk_endpoint_1(self): |
There was a problem hiding this comment.
Instead of using 1,2,3 can we make these test names a bit clearer?
e.g.
test_sdk_endpoint_both_inputs_set
test_sdk_endpoint_legacy_set_with_builtin_data
test_sdk_endpoint_legacy_set_without_builtin_data
There was a problem hiding this comment.
I'm also just now realizing that builtin is overloaded here. We might want to change the method name for detecting default endpoints.json data. uses_default_endpoint_data?
There was a problem hiding this comment.
Tests are renamed in 87be3a7.
Re overloaded use of term "builtin": Unfortunately, both uses already have precedent. "Builtin data files" is consistent with botocore.loaders. BUILTIN_DATA_PATH and botocore.loaders.BUILTIN_EXTRAS_TYPES. (And of course the other use of "builtin" comes from the cross-SDK endpoints spec.)
This PR is part of the ongoing endpoint resolution v2 work, together with #2737, #2740, #2751, #2747. It contains the code that assigns values to a list of "builtin parameters" that may be referenced by endpoint rulesets. It's glue code between the existing
ClientCreatorandClientArgsCreatorand the futureEndpointProvider(see #2737).Q: By itself, this PR adds only unused code. Why is it a separate PR? — A: I decided to break this out of a larger PR with all the middleware for calling
EndpointProviderto keep this a separate review/discussion. Even though the code is pretty simple, there could be a complex discussion hiding behind each parameter assignment and how it is tested. Let's get these debates out of the way first :)