Add ToString and Default impls #7
Conversation
Signed-off-by: James Sturtevant <[email protected]>
| @@ -152,6 +156,13 @@ impl ToConfig for WasmConfig { | |||
| } | |||
| } | |||
|
|
|||
| impl ToString for WasmConfig { | |||
There was a problem hiding this comment.
Generally speaking, it isn't good practice to use serialization in ToString because of the overhead involved with serialization (plus the possibility of an error)
| @@ -128,6 +128,10 @@ impl WasmConfig { | |||
| annotations, | |||
| } | |||
| } | |||
|
|
|||
| pub fn to_string_pretty(&self) -> anyhow::Result<String> { | |||
There was a problem hiding this comment.
I feel like it would be easier for consumers to just use the serialization tool of their choice to get a string here. Based on what I see in your runwasi PR here, maybe the OciConfig trait could have a to_json method that encapsulates this instead
| @@ -18,7 +18,7 @@ pub trait ToConfig { | |||
| } | |||
|
|
|||
| /// The config type struct for `application/wasm` | |||
| #[derive(Serialize, Deserialize, Debug)] | |||
| #[derive(Serialize, Deserialize, Debug, Default)] | |||
There was a problem hiding this comment.
So I actually avoided Default on purpose because the layer_digests needs to be set with the content of whatever this config is based on and should always be tied to a specific component (hence the loader functions). This was mostly to avoid people shooting themselves in the foot by not setting things on the config properly so it could just be my paranoia. If we want to go down this route, there are 2 ways we could handle this though:
- Manually implement default and make sure the
architecturefield is set and theosfield defaults to wasip2. - Create a
newfunction that creates and empty one and takesosas a parameter so people can set it. You could also havenew_component_configandnew_module_configthat are wrappers that just set theosfield for you correctly.
With that said however, based on what I saw in the linked runwasi PR:
let mut builder = Builder::<WasmConfig>::default();
Could it just be better for your use case to do impl Default for Builder<WasmConfig>?
|
These comments make sense to me, I was initially following what was done in https://github.com/containers/oci-spec-rs but can add these implementations as suggested directly in runwasi 👍 Thanks for the feedback! |
Add a few implementations that make it easier to use as a library for creating the config object.
Found while adding support to generate packages in runwasi containerd/runwasi#631