-
Notifications
You must be signed in to change notification settings - Fork 862
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 overloads to GetAWSOptions that allow creating service specific client configs #2571
Added overloads to GetAWSOptions that allow creating service specific client configs #2571
Conversation
@@ -219,39 +219,37 @@ private static ClientConfig CreateConfig(Type serviceInterfaceType, AWSOptions o | |||
} | |||
|
|||
var defaultConfig = options.DefaultClientConfig; | |||
if (options.IsDefaultClientConfigSet) |
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 the one use of the internal IsDefaultClientConfigSet
which checks to see if the backing field of DefaultClientConfig
is null. If you read DefaultClientConfig
like is done above it sets the value so this is always true.
I went ahead and deleted the property in order to not have this gotcha sitting around.
var clientConfigTypeInfo = typeof(ClientConfig).GetTypeInfo(); | ||
foreach (var property in clientConfigTypeInfo.DeclaredProperties) | ||
var clientConfigTypeInfo = options.DefaultClientConfig.GetType(); | ||
var properties = clientConfigTypeInfo.GetProperties(BindingFlags.Public | BindingFlags.Instance); |
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.
I changed the uses of GetTypeInfo
to be GetType
+ GetProperties
with binding flags in order to get inherited properties as well as ones on the declaring type.
This is also probably good to do because GetTypeInfo
is possibly going to be deprecated as a result of this dotnet/runtime#53217
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.
Since options is gauranteed to have a value I agree that it's fine to use GetType()
here
why this PR is stuck in review since 2021. Pls pull this change its useful feature |
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.
I've reviewed these changes and everything works as expected. Thanks for the contribution! Will await @normj feedback before releasing.
I went ahead and resolved the merge conflicts as well. |
This was merged in NETCoreSetup v 3.7.7 |
This implements a solution to #1796 where client specific configuration can't be set using the .net core configuration extensions.
Description
The main user facing change is an added overload to
GetAWSOptions
that takes a generic parameter of the type of aws config you are wanting to use. This allows settings such asForcePathStyle
in a derived class fromClientConfig
to be set.Motivation and Context
This allows greater flexibility in configuring aws clients purely from
IConfiguration
sources. This resolves #1796Testing
Testing so far has been done through the unit tests for AWSOption and client configs.
Types of changes
Checklist
License