libvirt-run: Add --update-from-host #142
Conversation
![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 introduces an --update-from-host flag to libvirt-run to streamline upgrading from a local build. The changes are well-structured, adding a new target_transport option and propagating it through the system, including metadata and documentation. My review includes suggestions to improve command-line argument handling for robustness and consistency, and a minor point on code maintainability by avoiding a magic string.
crates/kit/src/install_options.rs
Outdated
| if let Some(t) = self.target_transport.as_deref() { | ||
| args.push(format!("--target-transport={t}")); | ||
| } |
There was a problem hiding this comment.
The argument for target_transport is being formatted as a single string "--target-transport={t}". This is inconsistent with how filesystem and root_size arguments are handled, which are pushed as two separate strings ("--filesystem", filesystem.clone()). For consistency and to avoid potential issues with how bootc parses arguments, it would be better to follow the existing pattern for options with values.
| if let Some(t) = self.target_transport.as_deref() { | |
| args.push(format!("--target-transport={t}")); | |
| } | |
| if let Some(ref t) = self.target_transport { | |
| args.push("--target-transport".to_string()); | |
| args.push(t.clone()); | |
| } |
crates/kit/src/libvirt/run.rs
Outdated
| #[clap(long)] | ||
| pub update_from_host: bool, |
There was a problem hiding this comment.
The --update-from-host flag implicitly sets --target-transport to containers-storage. If a user specifies both --update-from-host and a different --target-transport, the user-provided transport will be silently overridden. This could be confusing.
To prevent this, you can make the arguments mutually exclusive using clap's conflicts_with attribute. This will cause clap to error out if both are provided, making the behavior clearer to the user.
| #[clap(long)] | |
| pub update_from_host: bool, | |
| #[clap(long, conflicts_with = "target-transport")] | |
| pub update_from_host: bool, |
crates/kit/src/libvirt/run.rs
Outdated
|
|
||
| if opts.update_from_host { | ||
| opts.bind_storage_ro = true; | ||
| opts.install.target_transport = Some("containers-storage".to_owned()); |
There was a problem hiding this comment.
The string literal "containers-storage" is used here. To avoid using a magic string, consider defining it as a constant at the module level, for example: const UPDATE_FROM_HOST_TRANSPORT: &str = "containers-storage";. This improves maintainability and readability.
This streamlines the UX of upgrading from a local build; it's part of the idea of bootc-dev#86 but this is a lot smaller than that. Signed-off-by: Colin Walters <[email protected]>
cgwalters
force-pushed
the
install-target-imgref
branch
from
d0c785b to
82f8284
Compare
2 participants
This streamlines the UX of upgrading from a local build; it's part of the idea of #86 but this is a lot smaller than that.