Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime #127

dotansimha · 2023-11-14T09:44:08Z

github-actions · 2023-11-16T16:23:33Z

🐋 This PR was built and pushed to the following Docker images:

Docker Bake metadata

{
"conductor": {
  "containerimage.config.digest": "sha256:89060319fcf765939468f865fb03d91c5117a9b8437ce093e8a77797091ca0a5",
  "containerimage.descriptor": {
    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
    "digest": "sha256:9f1423b1835a2b37efe46358d0bcb7cd3c6223aa2444f5bbfd67a33470a5f270",
    "size": 901,
    "platform": {
      "architecture": "amd64",
      "os": "linux"
    }
  },
  "containerimage.digest": "sha256:9f1423b1835a2b37efe46358d0bcb7cd3c6223aa2444f5bbfd67a33470a5f270",
  "image.name": "ghcr.io/the-guild-org/conductor-t2/conductor:bf6ece8f9a84928ae4198b4b1118008aff1153c6"
}
}

ok separated config now wip wip wip ok ok ok it's working again ok getting there ok bring back plugins fix graphiql fix graphiql again fix tracing ok ok ok fix clippy, fmt, over-http and ci fix benches fix deps fix docker image fix

YassinEldeeb · 2023-11-19T01:47:53Z

crates/conductor/src/lib.rs

+        &mut |route_data, app, path| {
+            let child_router = Scope::new(path.as_str())
+                .app_data(web::Data::new(route_data))
+                .route("/.*", web::route().to(handler));


when I tried running locally, I got a few of this warning:

2023-11-19T01:32:35.930689Z WARN actix_router::resource: Tail segments must have names. Consider `.../{tail}*`. This may become a panic in the future. 2023-11-19T01:32:35.930853Z WARN actix_router::resource: Tail segments must have names. Consider `.../{tail}*`. This may become a panic in the future. 2023-11-19T01:32:35.930998Z WARN actix_router::resource: Tail segments must have names. Consider `.../{tail}*`. This may become a panic in the future.

In Actix-web, when defining a route that captures all the remaining path segments, we should give a name to these segments, which are referred in the error as the "tail" segment. The warning suggests that we're currently using a route that captures all the remaining segments but we don't name them.

Also, the warning mentions that this might become a panic in future versions of Actix-web.

btw, Actix-web lets us catch the trailing segments and access it in the handler like this:

async fn handler( req: HttpRequest, body: Bytes, route_data: web::Data<ConductorGatewayRouteData>, gw: web::Data<ConductorGateway>, path: web::Path<(String,)>, // Optionally added to capture the tail segment, in case we ever need it ) -> impl Responder { // .... let tail_segment = &path.0;

Suggested change

.route("/.*", web::route().to(handler));

.route("/{tail:.*}", web::route().to(handler));

Update: using named segments path breaks path matching, visiting: /graphql and /test doesn't work after the switch, maybe we should wait for a fix...I tried debugging it for some time, but I can't seem to get it to work without anonymous segments matching.

YassinEldeeb · 2023-11-19T01:53:24Z

.github/workflows/release.yaml

@@ -101,7 +138,7 @@ jobs:
          command: build
          target: ${{ matrix.platform.target }}
          args: "--locked --release"
-          strip: true
+          strip: false


I believe strip default value is false

Suggested change

strip: false

YassinEldeeb · 2023-11-19T02:26:46Z

crates/async_runtime/src/lib.rs

@@ -0,0 +1,28 @@
+use core::future::Future;


I'm not sure if async_runtime abstraction is needed in its own crate, it's very minimal.
and it's only to encounter cross platform runtime differences with wasm.

Also, it does handle reqwest client initiation, so the name async_runtime is kind of confusing.

But, also at the same time, I appreciate separating it, since this can grow much larger in the future as we progress, it also eases testing cross-platform compatibility, so how about the name wasm_polyfills instead?

and when it comes to usage, to be like this:

-let fetcher = create_http_client().build().unwrap(); +let fetcher = wasm_polyfills::create_http_client().build().unwrap();

So, we eliminate a few jumps to imports & implementation, when reading inline code usage.

YassinEldeeb · 2023-11-19T02:41:21Z

crates/benches/Cargo.toml

+[dependencies]
+criterion = { version = "0.5.1", features = ["html_reports"] }
+conductor = { path = "../conductor" }
+futures = { workspace = true }


I had no idea { workspace = true } was added to infer workspace root dependencies' versions
Last time I checked, it was an abandoned RFC 😄
rust-lang/rfcs#2906

This is really cool! Good to know!

YassinEldeeb · 2023-11-19T02:51:12Z

crates/cloudflare_worker/Cargo.toml

@@ -0,0 +1,21 @@
+[package]


I believe we can separate the workspace instead of:

crates ├── async_runtime ├── benches ├── cloudflare_worker ├── common ├── conductor ├── config └── engine

to:

bins ├── conductor └── cloudflare_worker libs ├── async_runtime ├── benches ├── common ├── config └── engine

So, that there is separation between what is executable and what is code specific like libraries. what do you think? 👀

YassinEldeeb · 2023-11-19T02:54:12Z

crates/cloudflare_worker/src/lib.rs

+    let conductor_config_str = env.var("CONDUCTOR_CONFIG").map(|v| v.to_string());
+
+    match conductor_config_str {
+        Ok(conductor_config_str) => match from_yaml(&conductor_config_str) {


I feel like from_yaml isn't clear when interpreted inline at the first glance,
how about we rename it to parse_config_from_yaml(..)?

YassinEldeeb · 2023-11-19T02:58:33Z

crates/config/src/lib.rs

+pub fn from_yaml(contents: &str) -> Result<ConductorConfig, serde_yaml::Error> {
+    serde_yaml::from_str::<ConductorConfig>(contents)
+}


since we added this utility, we can also use it above, and extract one for json too like this:

pub fn parse_config_from_yaml(contents: &str) -> Result<ConductorConfig, serde_yaml::Error> { serde_yaml::from_str::<ConductorConfig>(contents) } pub fn parse_config_from_json(contents: &str) -> Result<ConductorConfig, serde_json::Error> { serde_json::from_str::<ConductorConfig>(contents) }

and above, we use those utilities too:

match path.extension() { Some(ext) => match ext.to_str() { Some("json") => parse_config_from_json(&contents).expect("Failed to parse config file"), Some("yaml") | Some("yml") => { parse_config_from_yaml(&contents).expect("Failed to parse config file") } } }

YassinEldeeb · 2023-11-19T03:44:49Z

crates/conductor/Cargo.toml

+actix-web = "4.4.0"
+tracing = { workspace = true }
+tracing-subscriber = "0.3.18"
+openssl = { version = "0.10", features = ["vendored"] }


I wonder where are we using this? 🤔

YassinEldeeb · 2023-11-19T04:15:18Z

crates/conductor/src/main.rs

-#[tokio::main]
-async fn main() {
+#[actix_web::main]


YESSS! Can't wait to see how that impacts the bench numbers

YassinEldeeb

That is Incredible!
It's super clean 💯

YassinEldeeb · 2023-11-19T04:31:28Z

Since, the feedback is opinionated, and nothing is really crucial.
I've applied the feedback here #136, so you can checkout to the branch, try it,
and get a feel for it, and decide if you like to merge it or not.

We can merge this now!

dotansimha force-pushed the cargo-workspace branch from 856f8ab to 57fbf31 Compare November 14, 2023 09:46

dotansimha changed the title ~~restructure project as Cargo workspace~~ Restructure project as Cargo workspace Nov 14, 2023

dotansimha force-pushed the cargo-workspace branch from b78f51c to d378692 Compare November 15, 2023 14:22

dotansimha changed the title ~~Restructure project as Cargo workspace~~ Restructure project as Cargo workspace and drop direct dependency on a HTTP server Nov 16, 2023

dotansimha marked this pull request as ready for review November 16, 2023 16:34

dotansimha force-pushed the cargo-workspace branch from f1d275b to 3ed3a61 Compare November 16, 2023 16:34

dotansimha requested a review from YassinEldeeb November 16, 2023 16:34

restructure

85038cd

ok separated config now wip wip wip ok ok ok it's working again ok getting there ok bring back plugins fix graphiql fix graphiql again fix tracing ok ok ok fix clippy, fmt, over-http and ci fix benches fix deps fix docker image fix

dotansimha force-pushed the cargo-workspace branch from 3ed3a61 to 85038cd Compare November 16, 2023 16:42

compile and run conductor as WASM (for CloudFlare Worker) (#134)

bf6ece8

dotansimha changed the title ~~Restructure project as Cargo workspace and drop direct dependency on a HTTP server~~ Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime Nov 18, 2023

YassinEldeeb reviewed Nov 19, 2023

View reviewed changes

YassinEldeeb approved these changes Nov 19, 2023

View reviewed changes

YassinEldeeb merged commit 93612a3 into master Nov 19, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime #127

Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime #127

dotansimha commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023

YassinEldeeb Nov 19, 2023

YassinEldeeb left a comment

YassinEldeeb commented Nov 19, 2023 •

edited

Loading

	.route("/.*", web::route().to(handler));
	.route("/{tail:.*}", web::route().to(handler));

Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime #127

Restructure project as Cargo workspace and drop direct dependency on a HTTP server, and also CF Worker (wasm) runtime #127

Conversation

dotansimha commented Nov 14, 2023 • edited Loading

github-actions bot commented Nov 16, 2023 • edited Loading

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb Nov 19, 2023

Choose a reason for hiding this comment

YassinEldeeb left a comment

Choose a reason for hiding this comment

YassinEldeeb commented Nov 19, 2023 • edited Loading

dotansimha commented Nov 14, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb Nov 19, 2023 •

edited

Loading

YassinEldeeb commented Nov 19, 2023 •

edited

Loading