feat(deployer): add more deployment metadata #943
Conversation
|
I really like the |
There was a problem hiding this comment.
This is almost there @hseeberger ! Thanks a lot for starting this, it is an important project for our upcoming console.
|
So you suggest that we use rmp-serde instead of bincode? Fine for me, I‘ll take a look. Am Freitag, 2. Juni 2023 um 18:29, Iulian Barbu - notifications at github.com ***@***.***> schrieb:
@iulianbarbu commented on this pull request.
In deployer/src/handlers/mod.rs:
) -> Result<Json<shuttle_common::models::deployment::Response>> {
let service = persistence.get_or_create_service(&service_name).await?;
let id = Uuid::new_v4();
+ let data = todo!("Base64 decode body.archive");
@hseeberger , @oddgrd mentioned to me that we already use rmp-serde for shuttle-next. Have you managed to use bincode already? I think it is at least the more popular encoding/decoding solution in the Rust ecosystem. There are figures that support it: [1] and [2], but there is a limitation as well [3]. I still think it's worth using it for Shuttle. The self-describing aspect described at [3] shouldn't be a must for us.
[1] https://github.com/djkoloski/rust_serialization_benchmark#raw-data
[2] https://users.rust-lang.org/t/choice-of-format-for-serialisation/81339 - seems like rmp-serde brings some significant overhead when serializing Vec<u8> for a specific case of a user. I am concerned about this because we tend to fit the same usage description when sending our code source archive in the body. A bit concerning for a big scale down the line, sending that much self-describing data over the network, which is not really needed when deserializing a static type.
[3] bincode doesn't support dynamically typed deserialization. I think this shouldn't be a problem for us if we provide the static type information when deserializing, rather than using an approach similar to serde_json [4], which can infer the types by seeking through the input for key parts (" for the beginning of a string, { for the beginning of an object etc.)
[4] https://serde.rs/impl-deserialize.html
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
There was a problem hiding this comment.
This is great @hseeberger and @jonaro00 ! Good job with owning and delivering this. I would need to test it out locally a bit before approving but I'll leave some nits before of that (feel free to discount them).
There was a problem hiding this comment.
Thanks! The data is handled well for both old and new projects. The only thing I noticed is that when doing cargo-shuttle deployments list we do not get the git metadata in the table. I thought it would be useful to be shown there, otherwise this info will be hidden to the CLI users.
|
@iulianbarbu Adding a way to fetch this metadata was not in spec for this PR. What you describe should of course be added, but we can keep that in a separate issue. |
Indeed. Would be great to open an issue to track it then. 👍 |
Ref #941.