system-reinstall-bootc: add progress indication during install steps#2091
system-reinstall-bootc: add progress indication during install steps#2091judavi wants to merge 1 commit intobootc-dev:mainfrom
Conversation
github-actions
bot
added
the
area/system-reinstall-bootc
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the user experience by adding progress indicators for long-running operations. A spinner is now shown during the bootc_has_clean check, and a message is printed before starting the installation. The changes are well-implemented. I have one suggestion to improve robustness by replacing unwrap() with expect() to avoid potential panics.
judavi
force-pushed
the
1270
branch
2 times, most recently
from
beb4b07 to
227d2d0
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Sorry for the delay in review and thank you so much for your contribution!
crates/tests-integration/Cargo.toml
Outdated
| oci-spec = "0.9.0" | ||
| rand = "0.10" | ||
| rexpect = "0.7" | ||
| rexpect = "0.6" |
There was a problem hiding this comment.
Hmm, this looks like a likely unintentional change?
There was a problem hiding this comment.
That's correct. Thank you! I'm not that used to use Codespaces for development
|
|
||
| prompt::temporary_developer_protection_prompt()?; | ||
|
|
||
| println!("Starting bootc installation. This may take several minutes..."); |
| ); | ||
| spinner.set_message("Checking image capabilities..."); | ||
| spinner.enable_steady_tick(Duration::from_millis(150)); | ||
| let has_clean = bootc_has_clean(&opts.image)?; |
There was a problem hiding this comment.
Hmm wait wait...did we end up pulling the image here? I think we did...if so that seems like the real bug. I think we should break out a clearly distinct pull phase.
There was a problem hiding this comment.
You're right bootc_has_clean was calling podman run <image> which would have implicitly pulled the image if it wasn't already cached locally. Fixed in the latest push: bootc_has_clean is now pub(crate) and called explicitly from run() in main.rs, after pull_if_not_present completes. reinstall_command is now a pure command builder that takes has_clean: bool as a parameter with no I/O side effects. The flow is now three clearly distinct phases: pull → capability check (spinner) → install
jmarrero
left a comment
jmarrero
left a comment
There was a problem hiding this comment.
This is passing all the tests, the main issue here for me is just squashing the commits into one. Thank you so much for fixing this issue.
… bug Add a spinner during the capability check step and a status message before the long-running installation, addressing issue bootc-dev#1270. Also fix a latent bug: `bootc_has_clean` was called inside `reinstall_command`, which runs `podman run <image>` and would implicitly pull the image if not already cached locally. The flow is now three clearly distinct phases: 1. Pull phase: `pull_if_not_present` explicitly pulls the image first, with a message so the user knows what is happening. 2. Capability check phase: `bootc_has_clean` is called after the image is guaranteed to be local, with a spinner labeled "Checking image capabilities...". 3. Install phase: `reinstall_command` is now a pure command builder that takes `has_clean: bool` as a parameter with no I/O side effects. Fixes: bootc-dev#1270 Signed-off-by: Juan Gomez <1766933+judavi@users.noreply.github.com>
|
Thanks @jmarrero ! Commits are squashed in one now. |
3 participants
Add progress indication to the long-running steps of system-reinstall-bootc to address #1270.
This also fixes a latent bug raised in review:
bootc_has_cleanwas callingpodman run <image>which could implicitly pull the image if it wasn't already cached locally, with no clear indication to the user.The flow is now three clearly distinct phases:
bootc_has_cleanis called after the image is guaranteed to be local, with a spinner labeled "Checking image capabilities...".