-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add support for building Containerd OCI Artifacts #135
Conversation
Address containerd#108 This adds an experimental tool for creating a WASM OCI Artifacts based on the proposal in https://docs.google.com/document/d/11shgC3l6gplBjWF1VJCWvN_9do51otscAm0hBDGSSAc. Containerd and runwasi don't currently support the OCI artifact format that is generated. Signed-off-by: James Sturtevant <jstur@microsoft.com>
fe1ba77
to
1ab2072
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!
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'm late to this review because this is already merged, but consider these as improvement ideas for the next patch set...
"yml" => { | ||
builder.add_layer_with_media_type( | ||
&path, | ||
"application/vnd.wasm.component.config.v1+json".to_string(), |
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.
Mapping yml
files to json
media type? It would work in the other direction because yaml is a superset of json.
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 was a typo, it should have been +yaml`
builder.build(f).unwrap(); | ||
} | ||
|
||
/// Simple program to greet a person |
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.
Hmm, is this comment right?
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.
woops! cleaning up. this was left over from the example in https://docs.rs/clap/latest/clap/#example
let oci_digest = "sha256:".to_owned() + &dgst; | ||
|
||
let mut media_type = MediaType::ImageLayer; | ||
if !layer.1.is_empty() { | ||
media_type = MediaType::Other(layer.1.clone()); |
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.
Should the layer.1
media types be enums instead of strings? Or is the idea that we are just a postman for the types?
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.
since the various types for wasm aren't set in stone and the library might be used for other reasons, I was just passing those values through. Maybe in the future we might want to have stricter checks here?
.context("failed to build image configuration") | ||
.unwrap(); | ||
|
||
let full_image_name = args.repo.unwrap_or("localhost:5000".to_string()) + "/" + &args.name; |
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.
If you use a hardcoded default URL, you need to document it somewhere.
|
||
let p = out_dir.join(args.name + ".tar"); | ||
let f = File::create(p).unwrap(); | ||
builder.build(f).unwrap(); |
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.
Clean up the file instead of just panicking?
} | ||
|
||
let config = spec::ConfigBuilder::default() | ||
.entrypoint(vec!["".to_owned()]) |
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.
Why is the entrypoint set? This can be perfectly right, I'm just curious. :-)
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 don't think it is nessary, I'll drop it for now
@@ -1,13 +1,13 @@ | |||
## OCI Tar Builder | |||
|
|||
This is a library that can be used to build OCI tar archives. It is used by the `wasi-demo-app` crate to build the OCI tar archive that is used to run the demo app. | |||
The currently implementation is to support encapsulating the `wasi-demo-app` wasm module into an OCI tar. It may be possible to use this for other purposes, but that not currently a goal of the project. | |||
The currently implementation is to support encapsulating the `wasi-demo-app` wasm module as an OCI tar. |
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.
Typo: "currently" -> "current"
Address #108
This adds an experimental tool for creating a WASM OCI Artifacts based on the proposal in https://docs.google.com/document/d/11shgC3l6gplBjWF1VJCWvN_9do51otscAm0hBDGSSAc. Containerd and runwasi don't currently support the OCI artifact format that is generated.