[Default Configuration Part 3]: add defaults from defaults mode to the configuration resolution chain #2803
Conversation
.../resources/software/amazon/awssdk/codegen/lite/defaultsmode/defaults-mode-configuration.java
Outdated
Show resolved
Hide resolved
| .build(); | ||
|
|
||
| return configuration.toBuilder() |
There was a problem hiding this comment.
Can we save a reference to the builder, rather than repeatedly calling toBuilder()?
There was a problem hiding this comment.
Some of the methods requires certain option to be available in the configuration itself, e.g., resolveCredentials requires config.option(AwsClientOption.AWS_REGION). If we don't call .build(), it'll be null. The Builder class doesn't expose attributes. Not sure if it is intentional, so I didn't change it.
...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java
Show resolved
Hide resolved
...ore/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsDefaultClientBuilder.java
Show resolved
Hide resolved
| DefaultsMode finalDefaultsMode = defaultsMode; | ||
| log.debug(() -> "The resolved defaults mode is: " + finalDefaultsMode); |
There was a problem hiding this comment.
This is a ugly workaround forVariable used in lambda expression should be final or effectively final rule. LMK if you can think of a cleaner workaround.
There was a problem hiding this comment.
Yup, not a fan of that limitation. Let us do what we know is safe. :)
Nit: How about adjusting the wording to something like this:
Resolved {serviceName} client's AUTO configuration mode to {result}
There was a problem hiding this comment.
On second thought, should we try to refer to the precise client name, e.g., S3Client vs S3AsyncClient?
There was a problem hiding this comment.
I'm not sure if it's necessary because I don't think most customers use both sync and async clients for the same service.
zoewangg
force-pushed
the
zoewang/defaults-mode-3
branch
from
cc0d247 to
5dbb31b
Compare
zoewangg
force-pushed
the
zoewang/defaults-mode-3
branch
from
5dbb31b to
b749805
Compare
...ava/software/amazon/awssdk/codegen/lite/defaultsmode/DefaultsModeConfigurationGenerator.java
Show resolved
Hide resolved
.../resources/software/amazon/awssdk/codegen/lite/defaultsmode/defaults-mode-configuration.java
Show resolved
Hide resolved
| DefaultsMode finalDefaultsMode = defaultsMode; | ||
| log.debug(() -> "The resolved defaults mode is: " + finalDefaultsMode); |
There was a problem hiding this comment.
Yup, not a fan of that limitation. Let us do what we know is safe. :)
Nit: How about adjusting the wording to something like this:
Resolved {serviceName} client's AUTO configuration mode to {result}
|
Kudos, SonarCloud Quality Gate passed! |
* Add support to generate DefaultsMode and implement configuration resolution (#2781) * [Default Configuration Part 2]:Implement auto mode discovery (#2786) * Implement auto mode discovery * Fix tests on CodeBuild * Address feedback * Add comment and rename misnamed constant * [Default Configuration Part 3]: add defaults from defaults mode to the configuration resolution chain (#2803) * Wiring up configuration * Address comments * Add test * Update debug statement and add singleton for AttributeMap * Add tlsNegotiationTimeout (#2814) * [Default Configuration Part 5]: Move default configuration related classes to aws-core (#2816) * Move default configuration related classes to aws-core * Remove extra space and fix build * [Default Configuration Part 6]: apply default s3 us-east-1 regional setting (#2825) * Add s3 regional setting * Rename option name * Fix checkstyle * Update sdk-default-configuraiton.json * Add changelog entry
Motivation and Context
Description
DefaultConfigurationConfigurationwhich contains the default settings for each modeTesting
added unit tests
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsLicense