Refactor entrypoint logic to one location #219
Conversation
jsturtevant
force-pushed
the
refactor-wasm-entrypoint
branch
from
8631d3a to
1c1f546
Compare
| @@ -160,3 +179,125 @@ pub fn setup_prestart_hooks(hooks: &Option<oci_spec::runtime::Hooks>) -> Result< | |||
| } | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| mod oci_tests { | |||
There was a problem hiding this comment.
Nice to see more tests were added!
Signed-off-by: James Sturtevant <[email protected]>
jsturtevant
force-pushed
the
refactor-wasm-entrypoint
branch
from
1c1f546 to
7953ea0
Compare
Signed-off-by: James Sturtevant <[email protected]>
jsturtevant
force-pushed
the
refactor-wasm-entrypoint
branch
from
7953ea0 to
7d70f40
Compare
|
I think there could be some refactoring here to improve the way the spec is processed (this function is called a few times) but that can be done in a follow up if that works 👍 |
| if !args.is_empty() { | ||
| let start = args[0].clone(); | ||
| let mut iterator = start.split('#'); | ||
| let mut cmd = iterator.next().unwrap().to_string(); |
There was a problem hiding this comment.
I guess #_start is invalid module name?
There was a problem hiding this comment.
There is a little bit of a discussion in #102 where this was originally introduced.
My understanding is that this is really a runwasi specific seperator and could be anything (:: was suggested at one point). I scanned through https://github.com/WebAssembly/WASI but couldn't find anything that said # is valid or not
There was a problem hiding this comment.
I think that if this is a runwasi-specific thing, we could actually write down the semantics in how this should work and especially which entrypoint strings are valid and which not. Then we could validate the input string based on that. The benefit would be that other projects could use the same notation and semantics, and we could document the notation also for the users.
For example, should the following strings work (or not):
foo#bar#barfoo#bar#barfoo#foo#barfoo#foo#bar; -- %
There was a problem hiding this comment.
this is a good idea, are you ok with opening an issue to track this? This particular PR isn't introducing anything new, just moving some code around so we don't have the same logic in several different places.
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
This was part of the code that I was working on when I found #194. I saw it in https://github.com/containerd/runwasi/pull/156/files#diff-94b2ab211987ee0bd5d798ac422f3c3e169b69da929d6684874f72c347671bfeR49 as well so thought we could use it in one location.
There is a test that is marked as
[skip]included that could be enabled once #194 is resolved or I can remove it for now.