Skip to content

Add missing properties and polish the codegen path #4458

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.

Sign up for GitHub

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

Jump to bottom
Merged
sugmanue merged 5 commits into from
Sep 26, 2023
Merged

Add missing properties and polish the codegen path #4458

sugmanue merged 5 commits into from
Sep 26, 2023

Conversation

@sugmanue
Copy link
Contributor

@sugmanue sugmanue commented Sep 21, 2023

Motivation and Context

Add a missing property to the SdkServiceClientConfiguration class <SdkId>AuthSchemeProvider authSchemeProvider() and a different interface BuilderInternal that knows about SdkClientConfiguration and properly handles the "special" properties such as

  • endpointOverride
  • clientOverrideConfiguration

Modifications

Testing

  • Added unit tests to verify that we can round trip from the BuilderInternal class.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@sugmanue sugmanue requested a review from a team as a code owner September 21, 2023 17:28
sugmanue
sugmanue commented Sep 21, 2023
View reviewed changes
...ore/src/main/java/software/amazon/awssdk/core/client/config/ClientOverrideConfiguration.java
/**
* Adds the overridden values to the client configuration builder.
*/
public SdkClientConfiguration.Builder addOverridesToConfiguration(SdkClientConfiguration.Builder builder) {
Copy link
Contributor Author

@sugmanue sugmanue Sep 21, 2023

Choose a reason for hiding this comment

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

Moved (again) to a different util class to avoid the direct coupling between ClientOverrideConfiguration which is an "public" class and the SdkClientConfiguration which is an "internal" class.

@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-update-client-service-config2 branch 4 times, most recently from ffe4629 to f010563 Compare September 25, 2023 14:09
@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-update-client-service-config2 branch from f010563 to 48d3139 Compare September 25, 2023 15:52
cenedhryn
cenedhryn reviewed Sep 25, 2023
View reviewed changes
.../resources/software/amazon/awssdk/codegen/poet/model/serviceclientconfiguration-builder.java Outdated
}

public static class BuilderImpl implements BuilderInternal {
private final SdkClientConfiguration.Builder builder;
Copy link
Contributor

@cenedhryn cenedhryn Sep 25, 2023

Choose a reason for hiding this comment

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

Can we call this builder something that distinguishes it from this type? configBuilder, internalBuilder, delegatingBuilder?

Copy link
Contributor Author

@sugmanue sugmanue Sep 25, 2023

Choose a reason for hiding this comment

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

Sure, which one? I used BuilderImpl because I like that name, if you think another one is better please let me know which one.

Copy link
Contributor

@cenedhryn cenedhryn Sep 25, 2023

Choose a reason for hiding this comment

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

I meant the variable builder that represents the SdkClientConfiguration.Builder

Copy link
Contributor Author

@sugmanue sugmanue Sep 25, 2023

Choose a reason for hiding this comment

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

Sure, just tell me how to name it and I will make the change.

Copy link
Contributor

@cenedhryn cenedhryn Sep 25, 2023

Choose a reason for hiding this comment

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

Go with internalBuilder then?

@sugmanue sugmanue force-pushed the sugmanue/sra-plugins-update-client-service-config2 branch from 7268442 to 287cf62 Compare September 26, 2023 01:16
@sugmanue sugmanue merged commit 349a0e6 into feature/master/sra-identity-auth Sep 26, 2023
@sugmanue sugmanue deleted the sugmanue/sra-plugins-update-client-service-config2 branch September 26, 2023 18:18
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 26, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 4 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 418 Code Smells

84.5% 84.5% Coverage
3.9% 3.9% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@gosar gosar mentioned this pull request Sep 27, 2023
Merged
12 tasks
aws-sdk-java-automation added a commit that referenced this pull request Nov 4, 2025
@aws-sdk-java-automation
Merge pull request #4458 from aws/staging/d9811d65-b281-4386-821d-456…
46dabc7 
…2550be2b9

Pull request: release <- staging/d9811d65-b281-4386-821d-4562550be2b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cenedhryn cenedhryn cenedhryn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sugmanue @cenedhryn