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

Remove middleware code when generating the orchestrator exclusively #2723

Merged
merged 5 commits into from
May 25, 2023

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented May 23, 2023

Motivation and Context

This PR removes all the middleware code from generated clients when the smithy.runtime.mode flag is set to orchestrator. It also changes the aws-config crate to use the fluent client instead of the Smithy client for STS and SSO based credential providers.

The polly test no longer compiles in orchestrator mode with these changes since presigning directly references middleware code. This will get fixed in a later PR.


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

@smithy-lang smithy-lang deleted a comment from github-actions bot May 24, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti marked this pull request as ready for review May 24, 2023 18:50
@jdisanti jdisanti requested review from a team as code owners May 24, 2023 18:50
private val codegenScope = arrayOf(
"Arc" to RuntimeType.Arc,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Arc can come from preludeScope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not in the Rust prelude. We could make a separate commonScope or something though.

@@ -164,6 +169,7 @@ class CustomizableOperationGenerator(
.resolve("client::interceptors::SharedInterceptor"),
)

// TODO(enableNewSmithyRuntime): Centralize this struct rather than generating it once per operation
Copy link
Contributor

Choose a reason for hiding this comment

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

If this PR is merged to main first, we can remove this TODO in #2726

@jdisanti jdisanti enabled auto-merge May 25, 2023 16:46
@jdisanti jdisanti added this pull request to the merge queue May 25, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Merged via the queue into main with commit 3c68521 May 25, 2023
@jdisanti jdisanti deleted the jdisanti-sra-conditionally-remove-middleware branch May 25, 2023 17:38
ysaito1001 added a commit that referenced this pull request May 26, 2023
## Motivation and Context
Render `CustomizableOperation` once in the `customizable` module instead
of rendering it in every operation's `builders` module.

## Description
This PR is a follow-up on #2706. The previous PR ported
`CustomizableOperation` in the middleware to the orchestrator but it had
a flaw in that the type was rendered per every operation. This was
clearly a codegen regression because the code for
`CustomizableOperation` is repeated many times and is rendered even if
no customization is requested for an operation.

This PR attempts to fix that problem. Just like `CustomizableOperation`
in the middleware, we render the new `CustomizableOperation` once in the
`customize` module per crate. To accomplish that, the new
`CustomizableOperation` uses a closure, called `customizable_send`, to
encapsulate a call to a fluent builder's `send` method and invokes it
when its own `send` method is called.

~~As a side note, this PR will not take care of the following. We have
[a separate PR](#2723) to fix
them and once we've merged it to this PR, they should be addressed
automatically (there will be merge conflicts, though):~~
- ~~Removing `send_orchestrator_with_plugin` (which is why a type
parameter `R` is present in the code changes, but it'll soon go away)~~
- ~~Incorrect signature of a closure in orchestrator's
`CustomizableOperaton::map_request`~~

## Testing
No new tests added as it's purely refactoring. Confirmed `sra-test`
passed (except for the one that's known to fail).

----

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

---------

Co-authored-by: Yuki Saito <[email protected]>
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.

2 participants