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.

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

Suggestion: Share AppSource and ResolvedAppSource in some form #2073

Open
Mossaka opened this issue Nov 9, 2023 · 7 comments
Open

Suggestion: Share AppSource and ResolvedAppSource in some form #2073

Mossaka opened this issue Nov 9, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@Mossaka
Copy link
Contributor

Mossaka commented Nov 9, 2023

As a maintainer of the spin shim, I found these two structs particularly useful for running spin application either from the local filesystem or from oci, but we can't directly take a dependency on the spin-cli crate due to build.rs. In order to share more logic between the up command and the spin Engine implementation in the containerd-shim, I'd suggest to move these two structs to spin_app crate that we depend on. Thoughts?

See: deislabs/containerd-wasm-shims#164

@itowlson
Copy link
Contributor

itowlson commented Nov 9, 2023

I wouldn't want to see these in spin-app, which relates specifically to the runtime environment and is bound to spin-core and wasmtime, but agree they may be reusable (cloud-plugin has very similar logic and has the same goal of loading interchangeably from spin.toml or OCI reference). They are bound up with the load process and particularly with how the Spin UI resolves references (whether paths or OCI references), so maybe in loader, or in their own crate so as to be specific and limited about dependencies...?

@Mossaka
Copy link
Contributor Author

Mossaka commented Nov 9, 2023

Yeah that makes sense. I am fine with either solutions as long as they are shareable in some form because we can't depend on spin-cli.

Maybe we can create a new crate like spin-cli-lib and move as much logic from spin cli to it as possible. WDYT?

@Mossaka Mossaka changed the title Suggestion: Move AppSource and ResolvedAppSource from cli to spin_app crate Suggestion: Share AppSource and ResolvedAppSource in some form Nov 9, 2023
@itowlson
Copy link
Contributor

itowlson commented Nov 9, 2023

My feelings about that are very mixed.

On the one hand, yes, it could be a powerful enabler of consistent behaviour and experience across different Spin runtime hosts (for hosts that want to opt into Spin CLI-like experiences, of course).

On the other hand, it likely ends up linking to everything in the world, meaning that innocent helper tools that just want to resolve a --from flag end up dragging in the whole of the runtime (as cloud-plugin did until recently!). This can technically be avoided with features, but features are unreliable at avoiding unintended dependencies. But perhaps breaking out lighter-weight subsets could be deferred - the cli-lib crate could always re-export broken-out types just as spin-app now does.

Let's have a look and see if any existing crates feel like a good home for these types. Otherwise, I'd suggest creating a tiny new crate for them, with cli-lib as a longer term idea. Thoughts?

@Mossaka
Copy link
Contributor Author

Mossaka commented Nov 13, 2023

For the time being, I think it would be great to add them to the loader crate. I can do some work here.

@lann
Copy link
Collaborator

lann commented Nov 14, 2023

There isn't very much code in (Resolved)AppSource; could you clarify which part(s) of it would be useful to share?

@Mossaka
Copy link
Contributor Author

Mossaka commented Nov 14, 2023

I was hoping to refactor this section of the code out: https://github.com/deislabs/containerd-wasm-shims/blob/main/containerd-shim-spin/src/engine.rs#L266-L294

@lann
Copy link
Collaborator

lann commented Nov 15, 2023

ResolvedAppSource exists for spin up to be able to "partially load" an app just to make trigger-specific --help work properly. Since the shim (presumably) doesn't need to do that I would suggest skipping that step and going straight to the LockedApp. Then you only need the OciRegistry arm of that function: https://github.com/deislabs/containerd-wasm-shims/blob/81d24438921494684ea9aaf0c3573dde01bf20df/containerd-shim-spin/src/engine.rs#L283-L292

@vdice vdice added the enhancement New feature or request label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 📋 Investigating / Open for Comment
Development

No branches or pull requests

4 participants