Skip to content
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

[Default Configuration Part 6]: apply default s3 us-east-1 regional setting #2825

Conversation

zoewangg
Copy link
Contributor

@zoewangg zoewangg commented Nov 4, 2021

  • Add ServiceMetadataAdvancedOption to support configuring s3 use us-east-1 regional setting
  • Apply default s3 us-east-1 regional setting

@@ -40,6 +43,7 @@
private Region region;
private ProfileFile profileFile;
private String profileName;
private final Map<ServiceMetadataAdvancedOption<?>, Object> advancedOptions = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does AttributeMap.Builder work for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but the public API ServiceMetadataConfiguration would need to take AttributeMap or AttributeMap.Builder instead, which is not consistent with ClientOverrideConfiguration#advancedOptions

public Builder advancedOptions(Map<ServiceMetadataAdvancedOption<?>, ?> advancedOptions) {

Builder advancedOptions(Map<SdkAdvancedClientOption<?>, ?> advancedOptions);

Comment on lines 98 to 99
return config.advancedOption(ServiceMetadataAdvancedOption.S3_US_EAST_1_REGIONAL_ENDPOINT)
.filter(REGIONAL_SETTING::equalsIgnoreCase).isPresent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually directly-configured options are higher priority than environment variables and system properties. I know in this case it's actually a default instead of a directly configured option (the customer can't configure this directly, right?), but should we name it accordingly to clarify it's the default, not the actual setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. Customers can't configure this if they are using the SDK client. I'll rename it to reflect that it's the default.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 5, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

97.6% 97.6% Coverage
0.0% 0.0% Duplication

@zoewangg zoewangg merged commit d54ec15 into feature/master/defaults-mode Nov 8, 2021
@zoewangg zoewangg deleted the zoewang/defaultsmode-supportS3RegionalSetting branch November 8, 2021 19:22
zoewangg added a commit that referenced this pull request Dec 17, 2021
* 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
aws-sdk-java-automation added a commit that referenced this pull request Dec 4, 2023
…e97599d25

Pull request: release <- staging/2800174d-5f20-472f-8765-cd6e97599d25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants