-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat(shim): support archive layers #121
Conversation
4a24750
to
acb11b9
Compare
1d14ea7
to
42df2a3
Compare
42df2a3
to
4a5e1da
Compare
4a5e1da
to
3f3d291
Compare
In 8e83775 I added a new |
2a331be
to
8e83775
Compare
Signed-off-by: Vaughn Dice <[email protected]>
… testing Signed-off-by: Vaughn Dice <[email protected]>
8e83775
to
bfae3fb
Compare
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.
LGTM!
@@ -40,10 +40,6 @@ const SPIN_HTTP_LISTEN_ADDR_ENV: &str = "SPIN_HTTP_LISTEN_ADDR"; | |||
const RUNTIME_CONFIG_PATH: &str = "/runtime-config.toml"; | |||
/// Describes an OCI layer with Wasm content | |||
const OCI_LAYER_MEDIA_TYPE_WASM: &str = "application/vnd.wasm.content.layer.v1+wasm"; |
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.
suggestion: make spin_oci::client::WASM_LAYER_MEDIA_TYPE
public and use it in the Spin Shim.
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.
In fact, Spin itself is waiting on the canonical type/string upstream: https://github.com/fermyon/spin/blob/main/crates/oci/src/client.rs#L34-L35. To be defined in https://github.com/bytecodealliance/registry perhaps? Therefore expecting both Spin and the shim would pull this from the upstream crate.
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.
LGTM!
// which will fall back to /tmp if TMPDIR is not set. /tmp is either not found or not accessible | ||
// in the shim environment, hence setting to current working directory. | ||
if env::var("TMPDIR").is_err() { | ||
log::debug!( |
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.
When do we expect TMPDIR
to be set? I am wondering if this log is necessary.
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 point... it appears it isn't ever set, currently. Glad to remove this debug log if you'd like.
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.
I think we would want to leave it, otherwise we might overwrite it. I don't know what scenario it would be set in, but if it was set and we overwrite I don't think that would be an expected behavior?
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.
LGTM
Thanks, all. Please merge when ready. |
Closes #28
Depends on fermyon/spin#2523
Draft for now as 2523 is based on Spin's main branch which has since bumped wasmtime to 21.0.1. Can either wait for that same bump to occur in this project (presumably in tandem with pinning to the next Spin release with said version) or can explore backporting 2523 to the v2.5 branch and cutting a patch release with it.
TODO