endpoint resolver/provider #3597
Conversation
kai-ion
force-pushed
the
noEnd
branch
3 times, most recently
from
2ace84b to
5e53be5
Compare
| @@ -78,6 +79,51 @@ namespace Endpoint | |||
| return SetFromClientConfiguration(static_cast<const Client::ClientConfiguration&>(config)); | |||
| } | |||
|
|
|||
| void BuiltInParameters::SetFromClientConfiguration(const Client::ClientConfiguration& config, const Aws::String& serviceName) | |||
| { | |||
| bool forceFIPS = false; | |||
There was a problem hiding this comment.
this logic is copy and pasted from the other function SetFromClientConfiguration with the exception that we handle the setting of override differently. lets not repeat ourselves and refactor this function so that we dont have copy and pasted code.
There was a problem hiding this comment.
Refactor the logic to use a shared SetFromClientConfigurationImpl function
| SetStringParameter(AWS_REGION, "region-not-set"); | ||
| } | ||
| } else if (!serviceName.empty()) { | ||
| Aws::Config::EndpointResolver::EndpointSource(serviceName, config.profileName, config.scheme, *this); |
There was a problem hiding this comment.
we shouldnt be passing *this, the objective here is figure out what the endpoint override it, we should not also be mutating self in this call. something along the lines of
OverrideEndpoint(
Aws::Config::EndpointResolver::EndpointSource(serviceName, config),
config.scheme);would be what we want to be doing
There was a problem hiding this comment.
Refactor endpointResolver to return an endpoint instead of mutating the config directly
| @@ -26,6 +26,8 @@ namespace Aws | |||
|
|
|||
| virtual void SetFromClientConfiguration(const Client::ClientConfiguration& config); | |||
| virtual void SetFromClientConfiguration(const Client::GenericClientConfiguration& config); | |||
| virtual void SetFromClientConfiguration(const Client::ClientConfiguration& config, const Aws::String& serviceName); | |||
There was a problem hiding this comment.
should the non - service name ones have a implementation? why would they not simply call the service name function but with a empty service name?
would this be breaking if someone subclasses BuiltInParameters and they were already overriding SetFromClientConfiguration?
There was a problem hiding this comment.
Refactor to use a fallback call instead of implementing new virtual functions
| /** | ||
| * Resolver that sources endpoints and sets them on endpoint providers. | ||
| */ | ||
| class AWS_CORE_API EndpointResolver |
There was a problem hiding this comment.
why do we have a class if all the methods are static and we have no members? currently this class is default constructible.
There was a problem hiding this comment.
Change this class into a name space instead
| /** | ||
| * Initialize client context parameters from a ClientConfiguration with service name | ||
| */ | ||
| virtual void InitBuiltInParameters(const ClientConfigurationT& config, const Aws::String& serviceName) = 0; |
There was a problem hiding this comment.
adding a new pure virtual method means that anything else implementing this will break. why not have a default implementation that calls the previous function to preserve backwards compat?
There was a problem hiding this comment.
Refactor to use a fallback call with empty service name
# Conflicts: # generated/src/aws-cpp-sdk-apptest/source/AppTestClient.cpp # generated/src/aws-cpp-sdk-iotfleethub/source/IoTFleetHubClient.cpp # generated/src/aws-cpp-sdk-lookoutmetrics/source/LookoutMetricsClient.cpp # generated/src/aws-cpp-sdk-lookoutvision/source/LookoutforVisionClient.cpp # generated/src/aws-cpp-sdk-qldb-session/source/QLDBSessionClient.cpp # generated/src/aws-cpp-sdk-qldb/source/QLDBClient.cpp # generated/src/aws-cpp-sdk-robomaker/source/RoboMakerClient.cpp
2 participants
Issue #, if available:
Description of changes:
Adding an endpoint resolver template to be called in client init to resolve endpoint in order chain
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.