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.

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

Implement the remaining part of supporting operation level configuration #2814

Merged
merged 19 commits into from
Jul 1, 2023
Merged

Implement the remaining part of supporting operation level configuration #2814

merged 19 commits into from
Jul 1, 2023

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Jun 28, 2023 •
edited
Loading

Motivation and Context

Implements the actual meat of config_override introduced as a skeleton in the past.

Description

This PR enables operation-level config via config_override on a customizable operation (see config-override.rs integration tests for example code snippets).

The way it's implemented is through ConfigOverrideRuntimePlugin. The plugin holds onto two things: a Builder passed to config_override and a FrozenLayer derived from a service config (the latter is primarily for retrieving default values understood by a service config). The plugin then implements the RuntimePlugin trait to generate its own FrozenLayer that contains operation-level orchestrator components. That FrozenLayer will then be added to a config bag later in the orchestrator execution in a way that it takes precedence over the client-level configuration (see register_default_runtime_plugins).

A couple of things to note:

  • The size of ConfigOverrideRuntimePlugin::config code-generated is getting large. Maybe each customization defines a helper function instead of inlining logic directly in config and we let the config method call those generated helpers.
  • The PR does not handle a case where retry_partition within config_override since it's currently #[doc(hidden)], e.g.
client
    .some_operation()
    .customize()
    .await
    .unwrap()
    .config_override(Config::builder().retry_partition(/* ... */))
    ...

Testing

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001 ysaito1001 marked this pull request as ready for review June 28, 2023 19:27
@ysaito1001 ysaito1001 requested review from a team as code owners June 28, 2023 19:27
@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti
jdisanti reviewed Jun 29, 2023
View reviewed changes
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

This is looking good. Some comments on making ServiceRuntimePlugin more efficient, and on testing.

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/CredentialCaches.kt Outdated Show resolved Hide resolved
aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/CredentialCacheConfigTest.kt Outdated Show resolved Hide resolved
aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/CredentialCacheConfigTest.kt Show resolved Hide resolved
@ysaito1001
Merge branch 'main' into ysaito/implement-full-config-override
cf567af
@ysaito1001
Update where ConfigOverrideRuntimePlugin is created
1dcfa38
@ysaito1001
Rename section RuntimePluginConfig to OperationConfigOverride
95318ea
@ysaito1001
Remove TODO for BuilderBuildExtras
decbd02 
This commit removes `TODO` for `BuilderBuildExtras` since the section is
still needed as long as we have other struct fields in `Config` other
than `FrozenLayer`, .e.g. `interceptors`.
@ysaito1001
Panic if only one of cache or provider is specified in `.config_overr…
a78489d 
…ide`

This commit addresses https://github.com/awslabs/smithy-rs/pull/2814/files#r1246815303
@ysaito1001
Update Kotlin unit tests
0e94e37 
This commit updates `config_override`-related unit tests written in
Kotlin so that the new way of testing should check the behavior of each
trait object retrieved from a config override layer. Previously, the
tests were written in a naive way in that it only checks whether the
appropriate orchestrator component was inserted into a config override
layer when a test calls a certain builder method.
@ysaito1001
Fix matching on ServiceConfig.OperationConfigOverride
9cc52cf 
This commit addresses #2814 (comment)
@ysaito1001 ysaito1001 requested a review from jdisanti June 30, 2023 03:47
@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti
jdisanti approved these changes Jun 30, 2023
View reviewed changes
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

This is awesome! Great work. Just one suggestion.

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsPresigningDecorator.kt Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@ysaito1001 ysaito1001 added this pull request to the merge queue Jul 1, 2023
@github-actions
Copy link

github-actions bot commented Jul 1, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Merged via the queue into main with commit d753827 Jul 1, 2023
40 of 41 checks passed
@ysaito1001 ysaito1001 deleted the ysaito/implement-full-config-override branch July 1, 2023 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdisanti jdisanti jdisanti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ysaito1001 @jdisanti @Velfi