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

add headers to dummy WSGI environment #5902

Merged
merged 2 commits into from Apr 21, 2022
Merged

add headers to dummy WSGI environment #5902

merged 2 commits into from Apr 21, 2022

Conversation

thrau
Copy link
Member

@thrau thrau commented Apr 21, 2022

This adds the commits that sprung from the reviews of #5876 that didn't make it into the PR because I force pushed from my old history on my laptop 🤡

@thrau thrau requested a review from alexrashed April 21, 2022 10:20
@thrau thrau temporarily deployed to localstack-ext-tests April 21, 2022 10:20 Inactive
@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 92.105% when pulling ca54ab0 on http-request-rework into f9aa2f5 on master.

@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   56m 55s ⏱️ -11s
   970 tests ±0     930 ✔️ ±0  40 💤 ±0  0 ±0 
1 186 runs  ±0  1 118 ✔️ ±0  68 💤 ±0  0 ±0 

Results for commit ca54ab0. ± Comparison against base commit f9aa2f5.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM! I only have a short question about one of the changes in the ASF parser tests - nothing blocking.
Great to see the environment being improved more and more. This will allow us to use more convenience features implemented in werkzeug!

@@ -878,9 +878,10 @@ def test_restxml_headers_parsing():
_botocore_parser_integration_test(
service="s3",
action="PutObject",
ContentLength=0,
Copy link
Member

Choose a reason for hiding this comment

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

q / nit: Are the changes in this test on purpose or more of a left-over from your testing when implementing the changes? I don't see a problem in leaving this here, just asking. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

this addition is actually necessary, since now we set the content-length header correctly in our Request class, and the content-length header is translated into a member field ContentLength by the parser: https://github.com/boto/botocore/blob/b7a6f50cca3dcfdab7022a8e1ebe9e31edf57581/botocore/data/s3/2006-03-01/service-2.json#L8641-L8645

@thrau thrau merged commit 378857c into master Apr 21, 2022
@thrau thrau deleted the http-request-rework branch April 21, 2022 16:48
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.

None yet

3 participants