-
Notifications
You must be signed in to change notification settings - Fork 965
Allow ServiceClientConfiguration.Builder to read from SdkClientConfiguration.Builder #4438
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
Allow ServiceClientConfiguration.Builder to read from SdkClientConfiguration.Builder #4438
Conversation
…s from SdkClientConfiguration.Builder
|
|
||
| builder.addMethod(buildClientMethod()); | ||
| builder.addMethod(initializeServiceClientConfigMethod()); | ||
| builder.addMethod(SyncClientBuilderClass.initializeServiceClientConfigMethod(serviceConfigClassName)); |
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.
Using a static method of SyncClientBuilderClass within AsyncClientBuilderClass seems ambigous. Would it be better to move the initializeServiceClientConfigMethod to a common utility class used by both sync/Async ?
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.
Yes, we can do that in a follow up pull request if that's OK.
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.
Please add a TODO : here so that we might not miss it.
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 will remove this method and instead move it to the base class in the next PR.
|
Do we already have functional junits test where these settings actually gets used/resolved ? |
Not yet, we will in the coming pull requests when those settings are actually used, right now I'm only setting them up for it. |
…gins-update-client-service-config
fbff43b to
afe4f64
Compare
|
SonarCloud Quality Gate failed.
|
| // advanced option | ||
| Signer signer = this.advancedOption(SdkAdvancedClientOption.SIGNER).orElse(null); | ||
| builder.option(SdkAdvancedClientOption.SIGNER, signer); | ||
| builder.option(SdkClientOption.SIGNER_OVERRIDDEN, signer != null); |
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 used to be
clientOverrideConfiguration.advancedOption(SIGNER).ifPresent(s -> {
builder.option(SIGNER_OVERRIDDEN, true);
});
which means SIGNER_OVERRIDDEN would be null or true before, but now is always false or true. This actually affects this logic:
Line 74 in 32390a2
| return isClientSignerOverridden.isPresent() || requestSigner.isPresent(); |
I think this can be fixed to check for true instead of isPresent.
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.
Fixing in #4473
…der_refresh1 Implement credentials refresh for LoginCredentialsProvider












Motivation and Context
This change allows the service generated instance of
ServiceClientConfiguration.Builderto read/write its values directly from anSdkClientConfiguration.Builderby mapping the required option to the getters and setters.Also this change adds two new properties to the
ServiceClientConfigurationclass to support the plugins use case:credentialsProviderofIdentityProvider<? extends AwsCredentialsIdentity>authSchemeProviderof<Service>AuthSchemeProvider(the service specific auth scheme provider)This change is part of the SRA plugins for Identity and Auth.
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License