Add support for group operations #60
Conversation
| @@ -20,3 +20,4 @@ thiserror = "1.0" | |||
| clap = "2.33" | |||
| git-version = "0.3.5" | |||
| command-group = "1.0.8" | |||
| async-recursion = "1.1.1" | |||
There was a problem hiding this comment.
No, please don't
Any recursion can be expanded to an iteration. But avoid async recursion because it dramatically consume memory and we don't want zinit to get killed.
There was a problem hiding this comment.
In general I would avoid recursion and definitely in async. The reason is each Future is a state machine that built during compile time. When u do recursion each state machine has a copy of itself, this means recursively this machine size has infinite size (unknown size in compile time) hence this is needed.
But I am 100% sure (even before i see where u need it) this can be rewritten in a way that doesn't need recursion
| Err(e) => { | ||
| if let Some(err) = e.downcast_ref::<io::Error>() { | ||
| if err.kind() != ErrorKind::NotFound { | ||
| return Err(e.context("failed to load service config")); | ||
| } | ||
| } else { | ||
| return Err(e.context("failed to load service config")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I prefer instead of using downcast (although it's fine) that instead we create our concrete defined error type (using thiserror for example)
I am wondering if it's better if you do stat first of the file to see if it exists instead of what you do here.
I am wondering also what will happen if you have
- file: <service>.yaml
- dir: <service>
what should take precedence ? should we monitor both, or only the group? do we give errors. etc..
| if let Some(err) = e.downcast_ref::<io::Error>() { | ||
| if err.kind() == ErrorKind::NotFound { | ||
| bail!( | ||
| "neither {}.yaml nor {} directory was found", | ||
| name.as_ref(), | ||
| name.as_ref() | ||
| ) | ||
| } |
There was a problem hiding this comment.
You can see this patterns starting to get out of hand and I think we need to define our own errors. Or completely avoid this by doing checks of which file/dir we need to process.
| if let Ok(ZInitStatus::Service(status)) = zinit.status(&service).await { | ||
| after.insert(service, format!("{:?}", status.state)); | ||
| } else { | ||
| after.insert(service, format!("{:?}", crate::zinit::State::Unknown)); | ||
| } |
There was a problem hiding this comment.
I think it was cleaner when we first get the status then format it. It's more readable before and also makes formatting in one place instead of 2
| } | ||
| } | ||
| Status::Group(result) => { | ||
| let mut start = true; |
There was a problem hiding this comment.
i think down is a better name for this variable. Since you assume this is true for all services. and if any service is not down or target is down but still running (pid != 0) then u set it to false.
Then check if down you call the start
Description
Add support for group operations. It parses directories under zinit configuration directory and add directory name as prefix to the grouped services. So, eventually they are just represented as normal services but different commands just handle how to do operations on these services as a whole.
Related issues