Create bindles from Spin apps #90

itowlson · 2022-02-20T22:33:29Z

This is still WIP - it works, but it needs the new bindle loader that hasn't merged to main yet, and is unpleasantly coupled to the loader internals. We'll discuss how to mitigate that but in the meantime here it is for people to play with!

itowlson · 2022-02-20T22:36:00Z

For those who want to have a go, the command is spin bindle push --app spin.toml (there is also a spin bindle prepare if you want to create a standalone bindle to gaze lovingly at). It uses your existing BINDLE_URL environment variable (or of course you can specify on the command line). There is no authentication. signing or 'skip TLS' at the moment.

radu-matei · 2022-02-21T11:56:03Z

Looking at the generated parcel for spin.toml — is the name correct here?

[[parcel]]
[parcel.label]
sha256 = "b3bfcb3d92b9c2663a4f9c81e52720e6a68957ff40cd2fd55894fb2d67e3187b"
mediaType = "application/vnd.fermyon.spin+toml"
name = "/private/var/folders/rs/7g5937rn0sq1vyrtwf0_48140000gn/T/.tmpvaCd7h/manifests/spin.b3bfcb3d92b9c2663a4f9c81e52720e6a68957ff40cd2fd55894fb2d67e3187b.toml"
size = 181

radu-matei · 2022-02-21T12:24:28Z

Bumping into the following serialization issue:

Error: Failed to expand 'spin.toml' to a bindle

Caused by:
    0: Failed to write app manifest to TOML
    1: values must be emitted before tables

Here is the full application manifest:

name = "fermyon.com"
apiVersion = "0.1.0"
version = "1.0.0"
description = "The Fermyon website running on Spin."
authors = [ "Fermyon Engineering <engineering@fermyon.com>" ]
trigger = {type = "http", base = "/" }


[[component]]
source = "spin-bartholomew.wasm"
id = "bartholomew"
files = [ "content/**/*" , "templates/*", "scripts/*", "config/*"]
[component.trigger]
route = "/..."

[[component]]
source = "ffs.wasm"
id = "fileserver"
files = ["static/**/*"]
environment = { PATH_PREFIX = "static/" }
[component.trigger]
route = "/static/..."

itowlson · 2022-02-21T20:22:50Z

Effing values must be emitted before tables. Usually means something was empty when I expected it not to be. I'll take a look - thanks!

Re the invoice - the name of the manifest parcel isn't used, but yeah, it would be better to omit it than to leak the name of the temp file it got staged to. Thanks!

itowlson · 2022-02-21T20:46:30Z

The Rust client requires us to provide a parcel name (even though the spec says it's optional). And the BindleWriter (which has been moderately battle tested as part of Hippo) currently relies on the name to locate the source file. So this might be a slightly fiddly fix.

itowlson · 2022-02-21T21:00:12Z

The other one is because serde's TOML implementation requires the schema struct to put all collections after all scalars in the struct source code. In our case, we had environment before files, and files in the Bindle manifest is a scalar.

radu-matei · 2022-02-22T04:23:12Z

Trying to push the template application after the latest changes (accommodating for the missing apiVersion field`):

➜ spin bindle push --app spin.toml
Error: Failed to push bindle to server

Caused by:
    Error pushing bindle to server: Error creating request

(this worked previously)

crates/loader/src/bindle/mod.rs

radu-matei · 2022-02-23T06:53:04Z

crates/loader/src/local/assets.rs

+            .with_context(|| format!("Failed to collect file mounts for {}", pattern))
+    });
+    let collections = results.into_iter().collect::<Result<Vec<_>>>()?;
+    let collection = collections.into_iter().flatten().collect();


Is there a way to avoid the three iterations over the patterns here?

There probably is, but I haven't found it. The into_iter on 59 is redundant though, as result is already an iterator. We might need to create a custom flatten_or_fail combinator which I think I had in the Hippo CLI but essentially did the same thing!

Could you elaborate on what you hope to achieve by doing so? If you're thinking about performance, the file accesses will absolutely dwarf the traversal. If it's clarity of expression then that's a good reason but let's just wrap the current code up in a combinator.

It was mainly clarity.
To be clear, this is by no means blocking.

crates/loader/src/local/mod.rs

crates/publish/src/bindle_pusher.rs

crates/publish/src/bindle_writer.rs

crates/publish/src/lib.rs

crates/publish/src/bindle_writer.rs

radu-matei · 2022-02-23T07:07:03Z

crates/publish/src/bindle_writer.rs

+impl BindleWriter {
+    pub async fn write(&self) -> anyhow::Result<()> {
+        // This is very similar to bindle::StandaloneWrite::write but... not quite the same
+        let bindle_id_hash = self.invoice.bindle.id.sha();


nit: is this variable binding used anywhere else?

Yeah, it's used on the next line.

(other than the next line that was)

No, that's the only place it's used... but that's enough isn't it? Not sure what you're getting at here, sorry.

Sorry, I should have been more clear to begin with.
As a personal preference, whenever there are variables that are only used once, I rethink whether there actually needs to be a variable binding there at all. In this case:

let bindle_dir = self.dest_dir.join(invoice.bindle.id.sha());

This is very nitpicky, don't feel like you have to take this into account.

SO MANY PHILOSOPHICAL RIFTS

crates/publish/src/expander.rs

radu-matei · 2022-02-23T07:25:29Z

crates/publish/src/lib.rs

+
+pub use bindle_pusher::push_all;
+pub use bindle_writer::write;
+pub use expander::expand_app_manifest;


Given Bindle is the only implementation for this crate, would it make sense to drop the bindle_ prefix from the files? And if we do that, how about just push, write, expand instead of the nouns?
(this is just a personal preference.)

src/bin/spin.rs

src/commands/bindle.rs

radu-matei

Manually tested with a rather complicated application, and everything seems to work great!

I left a bunch of comments, most of them rather nitpicky, and definitely nothing blocking.
This is awesome, thank you!

crates/publish/src/expander.rs

radu-matei

Yaaay, this looks great, thank you SO much!

LGTM

radu-matei mentioned this pull request Feb 22, 2022

feat: add Bindle loader #94

Merged

1 task

itowlson force-pushed the bindle-publish branch 2 times, most recently from 76cc930 to 2da0d0a Compare February 23, 2022 04:17