-
Notifications
You must be signed in to change notification settings - Fork 198
reinstall: Only pull the image if it's not already present #1355
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,12 +96,35 @@ pub(crate) fn reinstall_command(image: &str, ssh_key_file: &str) -> Result<Comma | |
| Ok(command) | ||
| } | ||
|
|
||
| pub(crate) fn pull_image_command(image: &str) -> Command { | ||
| fn pull_image_command(image: &str) -> Command { | ||
| let mut command = Command::new("podman"); | ||
| command.args(["pull", image]); | ||
| command | ||
| } | ||
|
|
||
| fn image_exists_command(image: &str) -> Command { | ||
| let mut command = Command::new("podman"); | ||
| command.args(["image", "exists", image]); | ||
| command | ||
| } | ||
|
|
||
| pub(crate) fn pull_if_not_present(image: &str) -> Result<()> { | ||
| let result = image_exists_command(image).status()?; | ||
|
|
||
| if result.success() { | ||
| println!("Image {} is already present locally, skipping pull.", image); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we need this, feels like a "silence is golden" type of thing? |
||
| return Ok(()); | ||
| } else { | ||
| println!("Image {} is not present locally, pulling it now.", image); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I like brevity too much but how about just:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm usually in favor of brevity too but my reasoning for this is since system-reinstall-bootc is supposed to be a user friendly tool then being super clear about what is happening might be right. Not a strong opinion if you think less output is better
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong opinion either honestly on this specific one I do though have a more strongly held opinion that at some point we probably want to make the whole output here more consistent/formatted. One possibility here is basically how Github Actions/Ansible and other things do it with clearly delineated "tasks" that have their own description.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filed this to track: #1356 |
||
| println!(); | ||
| pull_image_command(image) | ||
| .run_with_cmd_context() | ||
| .context(format!("pulling image {}", image))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Path to the podman installation script. Can be influenced by the build | ||
| /// SYSTEM_REINSTALL_BOOTC_INSTALL_PODMAN_PATH parameter to override. Defaults | ||
| /// to /usr/lib/system-reinstall-bootc/install-podman | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ use std::{ | |
|
|
||
| use crate::install; | ||
|
|
||
| const TIMEOUT: u64 = 120000; | ||
| const TIMEOUT: u64 = 60000; | ||
|
|
||
| fn get_deployment_dir() -> Result<std::path::PathBuf> { | ||
| let base_path = Path::new("/ostree/deploy/default/deploy"); | ||
|
|
@@ -62,11 +62,14 @@ pub(crate) fn run(image: &str, testargs: libtest_mimic::Arguments) -> Result<()> | |
| )?; | ||
|
|
||
| // Basic flow stdout verification | ||
| p.exp_string( | ||
| format!("Image {image} is already present locally, skipping pull.").as_str(), | ||
| )?; | ||
| p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?; | ||
| p.exp_string("Would you like to import its SSH authorized keys")?; | ||
| p.exp_string("into the root user on the new bootc system?")?; | ||
| p.exp_string("Then you can login as root@ using those keys. [Y/n]")?; | ||
| p.send_line("a")?; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was "a" preexisting bug?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sort of, "a" still works because |
||
| p.send_line("y")?; | ||
|
|
||
| p.exp_string("Going to run command:")?; | ||
|
|
||
|
|
@@ -133,13 +136,35 @@ pub(crate) fn run(image: &str, testargs: libtest_mimic::Arguments) -> Result<()> | |
| )?; | ||
|
|
||
| p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?; | ||
| p.send_line("a")?; | ||
| p.exp_string("[Y/n]")?; | ||
| p.send_line("y")?; | ||
| p.exp_string("NOTICE: This will replace the installed operating system and reboot. Are you sure you want to continue? [y/N]")?; | ||
| p.send_line("y")?; | ||
| p.exp_string("Insufficient free space")?; | ||
| p.exp_eof()?; | ||
| Ok(()) | ||
| }), | ||
| Trial::test("image pull check", move || { | ||
| let sh = &xshell::Shell::new()?; | ||
| install::reset_root(sh, image)?; | ||
|
|
||
| // Run system-reinstall-bootc | ||
| let mut p: PtySession = rexpect::spawn( | ||
| "/usr/bin/system-reinstall-bootc quay.io/centos-bootc/centos-bootc:stream10", | ||
| Some(600000), // Increase timeout for pulling the image | ||
| )?; | ||
|
|
||
| p.exp_string("Image quay.io/centos-bootc/centos-bootc:stream10 is not present locally, pulling it now.")?; | ||
| p.exp_regex("Found only one user ([^:]+) with ([\\d]+) SSH authorized keys.")?; | ||
| p.exp_string("[Y/n]")?; | ||
| p.send_line("y")?; | ||
| p.exp_string("NOTICE: This will replace the installed operating system and reboot. Are you sure you want to continue? [y/N]")?; | ||
| p.send_line("y")?; | ||
| p.exp_string("Operation complete, rebooting in 10 seconds. Press Ctrl-C to cancel reboot, or press enter to continue immediately.")?; | ||
| p.send_control('c')?; | ||
| p.exp_eof()?; | ||
| Ok(()) | ||
| }), | ||
| ]; | ||
|
|
||
| libtest_mimic::run(&testargs, tests.into()).exit() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.