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

use rolo twisted gateway integration #10428

Merged
merged 4 commits into from Mar 13, 2024
Merged

use rolo twisted gateway integration #10428

merged 4 commits into from Mar 13, 2024

Conversation

thrau
Copy link
Member

@thrau thrau commented Mar 11, 2024 •
edited

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 GitHub Actions
Copy link

github-actions bot commented Mar 11, 2024 •
edited

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 GitHub Actions
Copy link

github-actions bot commented Mar 12, 2024 •
edited

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

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

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
31 checks passed
@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.

None yet
3 participants
@thrau @alexrashed @bentsku