Re: [pve-devel] [PATCH installer v6 36/36] autoinst-helper: add prepare-iso subcommand

[Aaron Lauterer <a.lauterer@proxmox.com>]

On  2024-04-18  10:48, Christoph Heiss wrote:
> Just quick three notes inline; nits other than the crate thing.
> Did not review in depth, LGTM overall tho.
> 
> On Wed, Apr 17, 2024 at 02:31:08PM +0200, Aaron Lauterer wrote:
> [..]
>> diff --git a/proxmox-autoinst-helper/Cargo.toml b/proxmox-autoinst-helper/Cargo.toml
>> index 2a88c0f..75399e0 100644
>> --- a/proxmox-autoinst-helper/Cargo.toml
>> +++ b/proxmox-autoinst-helper/Cargo.toml
>> @@ -19,3 +19,4 @@ serde_json = "1.0"
>>   toml = "0.7"
>>   log = "0.4.20"
>>   regex = "1.7"
>> +which = "4.2.5"
> 
> Misses the debian/control entry, but see also below.
> 
thanks for catching that, more further down

> [..]
>>
>> +fn prepare_iso(args: &CommandPrepareISO) -> Result<()> {
>> +    check_prepare_requirements(args)?;
>> +
>> +    if args.install_mode == AutoInstModes::Included && args.answer_file.is_none() {
>> +        bail!("Missing path to answer file needed for 'direct' install mode.");
>> +    }
>> +    if args.install_mode == AutoInstModes::Included && args.cert_fingerprint.is_some() {
>> +        bail!("No certificate fingerprint needed for direct install mode. Drop the parameter!");
>> +    }
>> +    if args.install_mode == AutoInstModes::Included && args.url.is_some() {
>> +        bail!("No URL needed for direct install mode. Drop the parameter!");
>> +    }
> 
>    if args.install_mode == AutoInstModes::Included {
>      if args.answer_file.is_none() {
>          bail!("Missing path to answer file needed for 'direct' install mode.");
>      }
>      if args.cert_fingerprint.is_some() {
>          bail!("No certificate fingerprint needed for direct install mode. Drop the parameter!");
>      }
>      if args.url.is_some() {
>          bail!("No URL needed for direct install mode. Drop the parameter!");
>      }
>    } else if (args.install_mode == AutoInstModes::Partition) { .. }
> 
> .. maybe, to avoid the repeated condition? (The resulting visual
> grouping is also nice)

good points

> 
> 
>> +    if args.answer_file.is_some() && args.install_mode != AutoInstModes::Included {
>> +        bail!("Set '-i', '--install-mode' to 'included' to place the answer file directly in the ISO.");
>> +    }
>> +    if args.install_mode == AutoInstModes::Partition && args.cert_fingerprint.is_some() {
>> +        bail!("No certificate fingerprint needed for partition install mode. Drop the parameter!");
>> +    }
>> +    if args.install_mode == AutoInstModes::Partition && args.url.is_some() {
>> +        bail!("No URL needed for partition install mode. Drop the parameter!");
>> +    }
>> +
> [..]
>> +
>>   fn get_disks() -> Result<BTreeMap<String, BTreeMap<String, String>>> {
>>       let unwantend_block_devs = vec![
>>           "ram[0-9]*",
>> @@ -335,3 +510,53 @@ fn get_udev_properties(path: &PathBuf) -> Result<String> {
>>       }
>>       Ok(String::from_utf8(udev_output.stdout)?)
>>   }
>> +
>> +fn parse_answer(path: &PathBuf) -> Result<Answer> {
>> +    let mut file = match fs::File::open(path) {
>> +        Ok(file) => file,
>> +        Err(err) => bail!("Opening answer file '{}' failed: {err}", path.display()),
>> +    };
>> +    let mut contents = String::new();
>> +    if let Err(err) = file.read_to_string(&mut contents) {
>> +        bail!("Reading from file '{}' failed: {err}", path.display());
>> +    }
> 
> There is also std::fs::read_to_string() for exactly that; and would
> avoid the whole open/close dance :^)
> 
> (Seems I missed that when reviewing the patch that introduced
> validate_answer())

ah yeah, I can change that in a follow up

> 
>> +    match toml::from_str(&contents) {
>> +        Ok(answer) => {
>> +            println!("The file was parsed successfully, no syntax errors found!");
>> +            Ok(answer)
>> +        }
>> +        Err(err) => bail!("Error parsing answer file: {err}"),
>> +    }
>> +}
>> +
>> +fn check_prepare_requirements(args: &CommandPrepareISO) -> Result<()> {
>> +    match which("xorriso") {
> 
> Do we really need _yet another_ crate dependency for that? Below is a
> check / bail! anyway when running the command proper.
> 
> And if we really want a explicit check beforehand, I'd just do something
> like
> 
>    fn which(name: &str) -> Result<()> {
>        match Command::new(name).output() {
>            Ok(_) => Ok(()),
>            Err(err) => Err(err.into()),
>        }
>    }

I really would like to have an explicit check and nice warning if it 
isn't present to reduce friction for users.
Initially, I thought of doing something like that, but considered it a 
bit too hacky.
But the "do we want an additional crate" argument could be reason enough 
to switch it over to a test like this.

> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel