system-reinstall-bootc: Handle --help

I wanted to add a CLI option here to avoid the reboot, and
ran into the fact that our option parsing was suboptimal to
start with.

We never documented `BOOTC_REINSTALL_CONFIG` at all...I'm
kind of tempted to deprecate it.

Signed-off-by: Colin Walters <[email protected]>

Author: cgwalters

crates/system-reinstall-bootc/src/config/mod.rs

@@ -1,52 +1,31 @@
-use anyhow::{ensure, Context, Result};
-use clap::Parser;
+use std::{fs::File, io::BufReader};
+
+use anyhow::{Context, Result};
+use bootc_utils::PathQuotedDisplay;
 use serde::{Deserialize, Serialize};
 
 mod cli;
 
+/// The environment variable that can be used to specify an image.
+const CONFIG_VAR: &str = "BOOTC_REINSTALL_CONFIG";
+
 #[derive(Debug, Deserialize, Serialize)]
 #[serde(deny_unknown_fields)]
 pub(crate) struct ReinstallConfig {
     /// The bootc image to install on the system.
     pub(crate) bootc_image: String,
-
-    /// The raw CLI arguments that were used to invoke the program. None if the config was loaded
-    /// from a file.
-    #[serde(skip_deserializing)]
-    cli_flags: Option<Vec<String>>,
 }
 
 impl ReinstallConfig {
-    pub fn parse_from_cli(cli: cli::Cli) -> Self {
-        Self {
-            bootc_image: cli.bootc_image,
-            cli_flags: Some(std::env::args().collect::<Vec<String>>()),
-        }
-    }
-
-    pub fn load() -> Result<Self> {
-        Ok(match std::env::var("BOOTC_REINSTALL_CONFIG") {
-            Ok(config_path) => {
-                ensure_no_cli_args()?;
-
-                serde_yaml::from_slice(
-                    &std::fs::read(&config_path)
-                        .context("reading BOOTC_REINSTALL_CONFIG file {config_path}")?,
-                )
-                .context("parsing BOOTC_REINSTALL_CONFIG file {config_path}")?
-            }
-            Err(_) => ReinstallConfig::parse_from_cli(cli::Cli::parse()),
-        })
+    pub fn load() -> Result<Option<Self>> {
+        let Some(config) = std::env::var_os(CONFIG_VAR) else {
+            return Ok(None);
+        };
+        let f = File::open(&config)
+            .with_context(|| format!("Opening {}", PathQuotedDisplay::new(&config)))
+            .map(BufReader::new)?;
+        let r = serde_yaml::from_reader(f)
+            .with_context(|| format!("Parsing config from {}", PathQuotedDisplay::new(&config)))?;
+        Ok(Some(r))
     }
 }
-
-fn ensure_no_cli_args() -> Result<()> {
-    let num_args = std::env::args().len();
-
-    ensure!(
-        num_args == 1,
-        "BOOTC_REINSTALL_CONFIG is set, but there are {num_args} CLI arguments. BOOTC_REINSTALL_CONFIG is meant to be used with no arguments."
-    );
-
-    Ok(())
-}

crates/system-reinstall-bootc/src/main.rs

@@ -2,6 +2,7 @@
 
 use anyhow::{ensure, Context, Result};
 use bootc_utils::CommandRunExt;
+use clap::Parser;
 use rustix::process::getuid;
 
 mod btrfs;
@@ -13,19 +14,43 @@ pub(crate) mod users;
 
 const ROOT_KEY_MOUNT_POINT: &str = "/bootc_authorized_ssh_keys/root";
 
+/// Reinstall the system using the provided bootc container.
+///
+/// This will interactively replace the system with the content of the targeted
+/// container image.
+///
+/// If the environment variable BOOTC_REINSTALL_CONFIG is set, it must be a YAML
+/// file with a single member `bootc_image` that specifies the image to install.
+/// This will take precedence over the CLI.
+#[derive(clap::Parser)]
+struct Opts {
+    /// The bootc image to install
+    pub(crate) image: String,
+    // Note if we ever add any other options here,
+}
+
 fn run() -> Result<()> {
+    // We historically supported an environment variable providing a config to override the image, so
+    // keep supporting that. I'm considering deprecating that though.
+    let opts = if let Some(config) = config::ReinstallConfig::load().context("loading config")? {
+        Opts {
+            image: config.bootc_image,
+        }
+    } else {
+        // Otherwise an image is required.
+        Opts::parse()
+    };
+
     bootc_utils::initialize_tracing();
     tracing::trace!("starting {}", env!("CARGO_PKG_NAME"));
 
     // Rootless podman is not supported by bootc
     ensure!(getuid().is_root(), "Must run as the root user");
 
-    let config = config::ReinstallConfig::load().context("loading config")?;
-
     podman::ensure_podman_installed()?;
 
     //pull image early so it can be inspected, e.g. to check for cloud-init
-    podman::pull_if_not_present(&config.bootc_image)?;
+    podman::pull_if_not_present(&opts.image)?;
 
     println!();
 
@@ -41,8 +66,7 @@ fn run() -> Result<()> {
 
     prompt::mount_warning()?;
 
-    let mut reinstall_podman_command =
-        podman::reinstall_command(&config.bootc_image, ssh_key_file_path)?;
+    let mut reinstall_podman_command = podman::reinstall_command(&opts.image, ssh_key_file_path)?;
 
     println!();
     println!("Going to run command:");

crates/tests-integration/src/container.rs

@@ -39,13 +39,24 @@ pub(crate) fn test_bootc_install_config() -> Result<()> {
     drop(config);
     Ok(())
 }
+
+/// Previously system-reinstall-bootc bombed out when run as non-root even if passing --help
+fn test_system_reinstall_help() -> Result<()> {
+    let o = Command::new("runuser")
+        .args(["-u", "bin", "system-reinstall-bootc", "--help"])
+        .output()?;
+    assert!(o.status.success());
+    Ok(())
+}
+
 /// Tests that should be run in a default container image.
 #[context("Container tests")]
 pub(crate) fn run(testargs: libtest_mimic::Arguments) -> Result<()> {
     let tests = [
         new_test("bootc upgrade", test_bootc_upgrade),
         new_test("install config", test_bootc_install_config),
         new_test("status", test_bootc_status),
+        new_test("system-reinstall --help", test_system_reinstall_help),
     ];
 
     libtest_mimic::run(&testargs, tests.into()).exit()