cli: Only allow some arguments if composefs-backend is true #1990
Conversation
github-actions
bot
added
area/install
Currently the cli accepts `--insecure`, `--bootloader`, `--uki-addon` for ostree installs as well, which is not quite right as in the best case scenario they won't do anything and in the worst case scenario they might leave the install in undefined state Also, update the bootloader docs to specify systemd-boot is only available for composefs backend Xref: bootc-dev#1989 Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
cfs-options-fix
branch
from
4a28be2 to
9dcdc2f
Compare
![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 correctly restricts the --insecure, --bootloader, and --uki-addon CLI arguments to be used only with --composefs-backend. This is a good improvement as it leverages clap's validation capabilities, providing earlier and clearer feedback to the user. The removal of the now-redundant manual validation code simplifies the implementation. The accompanying documentation update is also helpful. I have one minor suggestion to further improve the clarity of the documentation.
| If bootupd is not present in the input container image, then systemd-boot will be used | ||
| by default (except on s390x). |
There was a problem hiding this comment.
This sentence could be slightly confusing. While it's true that systemd-boot is the fallback if bootupd is not present, for ostree-based installs this will result in an error. It might be clearer to explicitly state that this default behavior applies to composefs installs to avoid any ambiguity.
For example:
For composefs-based installations, if `bootupd` is not present in the input container image, then `systemd-boot` will be used by default (except on s390x).| If bootupd is not present in the input container image, then systemd-boot will be used | |
| by default (except on s390x). | |
| For composefs-based installations, if `bootupd` is not present in the input container image, then systemd-boot will be used | |
| by default (except on s390x). |
There was a problem hiding this comment.
Yeah, but not new with this PR
| #[clap(long, default_value_t, requires = "composefs_backend")] | ||
| #[serde(default)] | ||
| pub(crate) insecure: bool, |
There was a problem hiding this comment.
While we're here...(not part of this PR but as followup) I would like to propose renaming this to something like --allow-missing-fsverity or something.
|
|
||
| ## systemd-boot | ||
|
|
||
| NOTE: systemd-boot is only supported for Composefs Backend and not for Ostree |
There was a problem hiding this comment.
How about just
systemd-boot is only supported by the composefs backend, and not the (default) ostree backend.
| If bootupd is not present in the input container image, then systemd-boot will be used | ||
| by default (except on s390x). |
There was a problem hiding this comment.
Yeah, but not new with this PR
| #[context("Installing to disk")] | ||
| #[cfg(feature = "install-to-disk")] | ||
| pub(crate) async fn install_to_disk(mut opts: InstallToDiskOpts) -> Result<()> { | ||
| opts.validate()?; |
There was a problem hiding this comment.
I'm fine with this, however I think we could easily change this to be anyhow::ensure!() instead - yes it's double checking but probably worth doing.
2 participants
Currently the cli accepts
--insecure,--bootloader,--uki-addonfor ostree installs as well, which is not quite right as in the best case scenario they won't do anything and in the worst case scenario they might leave the install in undefined stateAlso, update the bootloader docs to specify systemd-boot is only
available for composefs backend
Xref: #1989