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
