Skip to content

use rolo twisted gateway integration #10428

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
thrau merged 4 commits into from
Mar 13, 2024
Merged

use rolo twisted gateway integration #10428

thrau merged 4 commits into from
Mar 13, 2024

Conversation

@thrau
Copy link
Member

@thrau thrau commented Mar 11, 2024 •
edited
Loading

Motivation

The coded added for twisted integration in #9834 moved to rolo in localstack/rolo#8, which also includes websocket support. This PR refactors our existing code to use the new rolo version.

Changes

  • Remove duplicate code from rolo code migration
  • Use rolo 0.4.0
  • localstack.http.asgi.ASGIWebSocket is removed from the API
  • added httpx as test library to test http2 support directly

TODO

What's left to do:

  • release rolo 0.4.0
  • upgrade rolo here
  • set default gateway back to hypercorn

@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Mar 11, 2024
@github-actions
Copy link

github-actions bot commented Mar 11, 2024 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   3m 18s ⏱️ +2s
393 tests ±0  342 ✅ ±0   51 💤 ±0  0 ❌ ±0 
786 runs  ±0  684 ✅ ±0  102 💤 ±0  0 ❌ ±0 

Results for commit 6034715. ± Comparison against base commit 5e21daa.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 12, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 32m 44s ⏱️ +48s
2 707 tests +2  2 451 ✅ +2  256 💤 ±0  0 ❌ ±0 
2 709 runs  +2  2 451 ✅ +2  258 💤 ±0  0 ❌ ±0 

Results for commit 6034715. ± Comparison against base commit 5e21daa.

♻️ This comment has been updated with latest results.

alexrashed added a commit that referenced this pull request Mar 12, 2024
@alexrashed
add upper limit for rolo
c7ef8c4 
rolo==0.4.0 contains some breaking changes addressed in
#10428.
This commit pins rolo to versions lower than 0.4.0.
This limit should be removed (or at least increased) in #10428.
@alexrashed
Copy link
Member

alexrashed commented Mar 12, 2024 •
edited
Loading

FYI: c7ef8c4 adds an upper limit to rolo to avoid the breaking changes, which means that this PR needs to be rebased on master after #10435, the limit has to be increased or removed, and the pins should maybe be re-generated.

Edit: Thanks @thrau for avoiding the pin at all in #10435.

@thrau thrau marked this pull request as ready for review March 13, 2024 13:04
@thrau thrau requested a review from bentsku March 13, 2024 13:04
@thrau
Copy link
Member Author

thrau commented Mar 13, 2024

the test run with twisted was successful, community against pro tests were failing due to #10444

bentsku
bentsku approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku 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 can see the line regarding the removal of localstack.http.asgi.ASGIWebSocket has been done in #10435 instead.

Nice cleanup 🧹 awesome to see things evolving!

localstack/aws/serving/twisted.py
Copy link
Contributor

@bentsku bentsku Mar 13, 2024

Choose a reason for hiding this comment

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

nice cleanup 👍

@thrau thrau merged commit a6d6a3c into master Mar 13, 2024
@thrau thrau deleted the rolo-twisted branch March 13, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bentsku bentsku bentsku approved these changes

Assignees

No one assigned

Labels

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thrau @alexrashed @bentsku