initial attempt to support rama in shuttle #1943
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Initial integration of the Rama framework into Shuttle, providing support for both transport-layer (TCP) and application-layer (HTTP) services using Rama's alpha release 0.2.0-alpha.4.
- Method name typo in
RamaService::trasnport()needs to be corrected totransport()inservices/shuttle-rama/README.md - Missing error handling for
.awaitcall inservices/shuttle-rama/src/lib.rsline 103 could lead to potential hangs - Documentation should clarify TLS termination capabilities and limitations within Shuttle's environment
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
This update adds implementation details for the Rama service integration, including service trait implementations and type wrappers for both transport and application layers.
- Added
RamaServicewrapper struct inservices/shuttle-rama/src/lib.rswith state management and service implementations - Implemented
shuttle_runtime::Servicetrait for both transport and application layer services - Added type aliases
ShuttleRamaTransportandShuttleRamaApplicationfor cleaner return types
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
This update adds example code and documentation for the Rama framework integration, demonstrating both Application and Transport service implementations.
- Added comprehensive examples in
services/shuttle-rama/README.mdshowing both HTTP and TCP service patterns - Updated version reference to Rama 0.2.0-alpha.5 in documentation and dependencies
- Missing GitHub example link in
codegen/src/lib.rsservice documentation table
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -24,6 +24,7 @@ mod shuttle_main; | |||
| /// | `ShuttleActixWeb` | [shuttle-actix-web](https://crates.io/crates/shuttle-actix-web)| [actix-web](https://docs.rs/actix-web/4.3) | 4.3 | [GitHub](https://github.com/shuttle-hq/shuttle-examples/tree/main/actix-web/hello-world)| | |||
| /// | `ShuttleAxum` | [shuttle-axum](https://crates.io/crates/shuttle-axum) | [axum](https://docs.rs/axum/0.7) | 0.7 | [GitHub](https://github.com/shuttle-hq/shuttle-examples/tree/main/axum/hello-world) | | |||
| /// | `ShuttlePoem` | [shuttle-poem](https://crates.io/crates/shuttle-poem) | [poem](https://docs.rs/poem/2.0) | 2.0 | [GitHub](https://github.com/shuttle-hq/shuttle-examples/tree/main/poem/hello-world) | | |||
| /// | `ShuttleRama` | [shuttle-rama](https://crates.io/crates/shuttle-rama) | [rama](https://docs.rs/rama/0.2.0-alpha.5) | |||
There was a problem hiding this comment.
style: Missing version and example columns that other services have in the table. Add version '0.2.0-alpha.5' and GitHub example link to maintain consistency.
services/shuttle-rama/README.md
Outdated
| async fn hello_world(mut stream: impl net::stream::Socket + net::Stream + Unpin) -> Result<(), Infallible> { | ||
| println!( | ||
| "Incoming connection from: {}", | ||
| stream | ||
| .peer_addr() | ||
| .map(|a| a.to_string()) | ||
| .unwrap_or_else(|_| "???".to_owned()) | ||
| ); |
There was a problem hiding this comment.
style: consider using debug! macro instead of println for better logging control in production
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the recent changes, I'll provide a summary focusing on the latest updates not covered in previous reviews:
Updated CircleCI configuration and package metadata to fully integrate the Rama service into Shuttle's build and deployment pipeline.
- Added shuttle-rama to CI/CD workflows in
.circleci/config.ymlfor testing and publishing - Configured Cargo.toml with appropriate metadata and rama dependency version 0.2.0-alpha.5
- Missing version and example columns in service documentation table need to be completed
Note: This summary focuses only on the CI/CD and metadata changes not mentioned in previous reviews, avoiding repetition of already-covered implementation and documentation points.
5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
services/shuttle-rama/Cargo.toml
Outdated
| keywords = ["shuttle-service", "rama"] | ||
|
|
||
| [dependencies] | ||
| rama = { version = "0.2.0-alpha.5", features = ["tcp", "http-full"] } |
There was a problem hiding this comment.
style: Using alpha version in production could be unstable. Consider adding version upper bound (<0.3.0) to prevent breaking changes
There was a problem hiding this comment.
alpha is best we have for now, in about a month or two we'll have 0.2.0, there's a warning for it in README
| const TEXT: &str = "Hello, Shuttle!"; | ||
|
|
||
| let resp = [ | ||
| "HTTP/1.1 200 OK", | ||
| "Content-Type: text/plain", | ||
| format!("Content-Length: {}", TEXT.len()).as_str(), | ||
| "", | ||
| TEXT, | ||
| "", | ||
| ] | ||
| .join("\r\n"); |
There was a problem hiding this comment.
style: consider extracting HTTP response construction into a separate function for better reusability and maintainability
| #[shuttle_runtime::async_trait] | ||
| impl<S, State, Response> shuttle_runtime::Service for RamaService<Application<S>, State> | ||
| where | ||
| S: rama::Service<State, rama::http::Request, Response = Response, Error = Infallible>, |
There was a problem hiding this comment.
logic: constraining Error to Infallible may be too restrictive for real-world HTTP services that need error handling
There was a problem hiding this comment.
You can use the Transport layer version if you want to have errors on transport layer, but an http service throwing errors makes no sense as only thing you can do with it is log + abort, which you can do yourself in your root http service. What you really want to do better however is turn it into an http error.
You can lookup in the rama docs, as for handlers, similar to Axum, you can also have Result types, but that's for your endpoint handlers not the root endpoint, but even there the Error type of the Result needs to be convertable into a Response.
| #[doc = include_str!("../README.md")] | ||
| pub type ShuttleRamaTransport<S, State = ()> = Result<RamaService<Transport<S>, State>, Error>; | ||
|
|
||
| #[doc = include_str!("../README.md")] | ||
| pub type ShuttleRamaApplication<S, State = ()> = Result<RamaService<Application<S>, State>, Error>; |
There was a problem hiding this comment.
style: including full README.md in type aliases via doc attribute seems excessive - consider more focused documentation
There was a problem hiding this comment.
this is copy pasted from other integrations found in services/
|
Hey @GlenDC! This looks good, thank you! The next step would be to create an example in https://github.com/shuttle-hq/shuttle-examples. You can test that against your service implementation by using a cargo patch, as described here: https://github.com/shuttle-hq/shuttle/blob/main/DEVELOPING.md#apply-patches, and then running it locally with For the remaining CI failure, it seems you just need to upgrade the shuttle-runtime version in the new crate to the latest 0.50.0. Regarding terminating TLS at the Rama service level, no, that is not currently possible, since we terminate TLS in our proxy that sits in front of all projects. Note that we may be delayed in releasing new versions of this crate if you have very frequent releases of Rama, but it seems like you're almost out of alpha (congratulations!), so we should be good! PS. apologies for our friend greptile, it can be a bit overeager! |
|
Thanks, I'll update that version and add those examples in a sibling PR somewhere this week. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the recent changes, I'll provide a summary focusing only on the latest updates not covered in previous reviews:
Updated Rama dependency version and runtime compatibility for the Shuttle integration.
- Upgraded rama dependency to version 0.2.0-alpha.6 in
services/shuttle-rama/Cargo.toml - Updated shuttle-runtime dependency to version 0.51.0 for compatibility
- Added "rama" to package keywords for better discoverability
Note: This summary focuses only on the dependency version updates and metadata changes not mentioned in previous reviews, avoiding repetition of already-covered implementation and documentation points.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the recent changes, I'll provide a summary focusing only on the latest updates not covered in previous reviews:
Updated Rama service implementation with improved error handling and state management.
- Added proper error propagation in
services/shuttle-rama/src/lib.rsfor both transport and application layer services - Implemented Clone and Debug trait derivations for
RamaServicewrapper types - Added state management functionality with
with_statemethod for both service types
Note: This summary focuses only on the implementation improvements not mentioned in previous reviews, avoiding repetition of already-covered points about dependencies, documentation, and CI/CD changes.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
@oddgrd examples added here: shuttle-hq/shuttle-examples#205 For now I'm not gonna add a MITM proxy example as the tls code of rama will receive some love prior to publishing 0.2.0, so will add more such examples once we are on a stable release. The code added now already in examples should be relatively stable already, and still show a lot already. In theory you can also port a lot more axum examples already, but didn't seem to fruitful to port them all. |
Description of change
Initial support of
ramahttps://ramaproxy.org/, using the latest alpha release0.2.0-alpha.5.First non-alpha release is expected Q1 2025, which will be
0.2.0.Rama can be used as a transport-layer runtime or application-layer runtime.
Rama users can also terminate their TLS themselves, but dunno if shuttle allows that?
Not a blocker if not (yet) possible, but just something that would at least have to be documented
in the
shuttle-ramaREADME.I also exposed the usual result public types, but similar to
tower,service types can become quite long due to the heavy use in nested generics.
As such probably I thought it would be easiest to just let them return
impl ShuttleServiceasthat keeps it clean, while still keeping rust happy and type-checked.
How has this been tested? (if applicable)
Not yet, please advice me on how to proceed.
It's my first real code contribution to shuttle so do let me know.