-
Notifications
You must be signed in to change notification settings - Fork 148
system-reinstall-bootc: Do not warn on unmounted LVM volumes #1663
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
Conversation
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.
Code Review
This pull request correctly prevents failures when encountering unmounted LVM volumes by ignoring errors from findmnt. My review includes a suggestion to improve robustness by logging these errors at a debug level instead of silently swallowing them. This helps in diagnosing unexpected issues without altering the intended behavior for unmounted volumes.
34e8b5f to
28cf283
Compare
|
Just ignore CS10, fedora 42 and 43 Looks CS9 still has issue: |
I'll clean up my uncommitted local changes that add a bunch of |
28cf283 to
44feb15
Compare
49b29bf to
8c11f90
Compare
Well I accidentally threw that away somewhere but easy enough to regenerate with claude, I just told it to add context annotations to every function in system-reinstall-bootc that returns a |
If the system has a swap partition (or any other volume which is not currently mounted) the `findmnt` command will (expectedly) fail to find it. Don't early exit in this case, instead just ignore that volume. If it wasn't mounted in the first place, we don't need to warn about it being unmounted after the reinstall operation is complete. Signed-off-by: John Eckersberg <[email protected]> Closes: bootc-dev#1659
…unctions Add #[context()] attribute macro to all functions that return Result to improve error reporting. This includes adding the fn-error-context dependency and importing the context macro in all relevant modules. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: John Eckersberg <[email protected]>
8c11f90 to
36328eb
Compare
|
We got Feel free to add bootc/tmt/plans/integration.fmf Line 7 in 24f2dd0
|
Ahh I suspect the bootc/crates/system-reinstall-bootc/src/lvm.rs Lines 27 to 28 in 24f2dd0
It's checking for the |
Signed-off-by: John Eckersberg <[email protected]>
|
Note that |
| .collect()) | ||
| } | ||
|
|
||
| #[context("check_root_siblings")] |
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.
These strings are intended to be somewhat human readable. Not a blocker but how about "Looking at root sibling mounts" or something?
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.
Yeah I just had claude generate context for all of the Result-returning functions in the crate so we would at least have some idea where to look for the problem. Not great but at least more useful than No such file or directory with absolutely no context at all.
If the system has a swap partition (or any other volume which is not
currently mounted) the
findmntcommand will (expectedly) fail tofind it. Don't early exit in this case, instead just ignore that
volume. If it wasn't mounted in the first place, we don't need to
warn about it being unmounted after the reinstall operation is
complete.
Signed-off-by: John Eckersberg [email protected]
Closes: #1659