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

content length enforcement would retroforwardly break tests #3523

Open
rcoh opened this issue Mar 27, 2024 · 0 comments
Open

content length enforcement would retroforwardly break tests #3523

rcoh opened this issue Mar 27, 2024 · 0 comments
Labels
bug Something isn't working client

Comments

@rcoh
Copy link
Collaborator

rcoh commented Mar 27, 2024

The problem

this is a "real" semver failure and it's kindof a mind bender: https://github.com/smithy-lang/smithy-rs/actions/runs/8457663420/job/23170159614#step:4:774

A journey:

  1. Content length enforcement is implemented via default runtime plugins. Default runtime plugins come in directly from the runtime crates. This is a cool emergent property that we can patch runtime behavior of the SDK without even releasing a new version of the SDK.
  2. I merged the content length enforcement PR after we cut the release. This caught a lot of tests setting the wrong content length including this one from Dynamo:

    smithy-rs/aws/sdk/integration-tests/dynamodb/tests/retries-with-client-rate-limiting.rs

    Line 29 in 745d48a

    .header("content-length", body.len().to_string())
  3. We are running the tests with the released SDK against the soon-to-be-released runtime libraries. Those tests have not been updated. Those tests fail against the new runtime libraries.

The way out

Disable this behavior. Release again to release all the fixed tests. Re-enable this behavior. This issue tracks re-enabling this behavior.
@rcoh rcoh mentioned this issue Mar 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 28, 2024
@rcoh
disable content length enforcement (#3524)
327e31e 
#3523

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@jdisanti jdisanti added client bug Something isn't working labels Apr 5, 2024
@jdisanti jdisanti mentioned this issue Apr 5, 2024
Open
@aajtodd aajtodd mentioned this issue Apr 30, 2024
Merged
1 task
github-merge-queue bot pushed a commit that referenced this issue May 13, 2024
@aajtodd
re-enable content length enforcement (#3618)
1117dc7 
## Motivation and Context
This PR re-enables content-length runtime plugin that was previously
disabled due to some erroneous tests. Those
[tests](https://github.com/awslabs/aws-sdk-rust/blob/main/sdk/dynamodb/tests/retries-with-client-rate-limiting.rs#L29)
appear to be fixed.
 
Original PR: #3491
Tracking issue: #3523


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working client
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jdisanti @rcoh