Conversation
There was a problem hiding this comment.
Code Review
This pull request successfully removes the custom Task abstraction, replacing it with direct usage of std::process::Command and helper extension traits from bootc_utils::CommandRunExt. This is a positive change that simplifies the codebase. The new implementation generally improves error reporting by capturing stderr on command failures. I've identified one minor regression where an error context attribute was removed, which I've suggested restoring to maintain detailed error diagnostics. Otherwise, the changes are excellent.
| } | ||
|
|
||
| #[context("Failed to wipe {dev}")] | ||
| pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> { |
There was a problem hiding this comment.
The #[context("Failed to wipe {dev}")] attribute was removed. This attribute provides valuable context to error messages when the wipefs command fails. Without it, the error message will be less specific, making debugging harder. It would be beneficial to restore this attribute to improve error diagnostics.
| pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> { | |
| #[context("Failed to wipe {dev}")] | |
| pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> { |
It wasn't a useful abstraction in the end for most cases. Signed-off-by: Colin Walters <walters@verbum.org>
958393d to
2425856
Compare
It wasn't a useful abstraction in the end for most cases.