-
Notifications
You must be signed in to change notification settings - Fork 13
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
Deploy from an OCI reference #105
Conversation
3740618
to
f5d04f4
Compare
The Spin crates are pointed to my fork because this depends on fermyon/spin#1825 - I couldn't see a way around needing this extra API, but I'm happy to take one if someone points out the blindingly obvious one I utterly missed. |
fafec82
to
c58a616
Compare
I believe this is ready for review, but am leaving as draft because the Cargo.toml Spin crate references can't be finalised until fermyon/spin#1825 merges. |
c58a616
to
f33945c
Compare
The required Spin change is merged: the Spin references now point to the main repo. Ready for review. |
/// Specifies to perform `spin build` before deploying the application. | ||
/// For local apps, specifies to perform `spin build` before deploying the application. | ||
/// | ||
/// This is ignored on remote applications, as they are already built. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For OCI deploys, do we need a "validate wasm" step? I can see a concern of a malicious wasm module being deployed in an OCI reference. Maybe we run the equivalent of wasm-tools component wit
to ensure that the Spin app has the imports and exports that we expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We don't currently do that for other Wasm files, e.g. those from URLs. How would you feel about punting that to a separate PR, possibly somewhere in the loader so it would work for local Spin too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also the wasm-tools component targets ...
command to ensure a component has the proper type. The diagnostics on error to explain the difference between actual and expected are pretty gnarly ATM but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itowlson I am okay with punting to another PR. On that note, @fibonacci1729 is this work planned for spin as part of the broader validation efforts? Maybe we create an issue to track it and potentially take this portion of it on soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Only had non-blocking Qs/thoughts.
Will we want to add the same reference -f
support to spin registry push
at some point?
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
f33945c
to
a0764a5
Compare
@vdice Sorry, I overlooked your question:
|
Yes, that was my thought. Not sure if a priority to support. Honestly, I was more thinking about consistency of |
This is VERY VERY DRAFT. The code needs a lot of tidying![:shipit: :shipit:](https://github.githubassets.com/images/icons/emoji/shipit.png)
; it doesn't currently validate OCI apps (for unsatisfied variables, nondefault stores, etc.); and I have broken the route printing. And probably other stuff too. But it worked, on my machine, at least once, soNOTES:
deploy
options for these if we wanted to - easy to implement, but more for users to understand.