Re: [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition
On 2024-03-29 12:43, Christoph Heiss wrote:
Mostly just some comments regarding the struct (member) definitions, to
make them (and their accompanying) checks a bit simpler.
On Thu, Mar 28, 2024 at 02:50:07PM +0100, Aaron Lauterer wrote:
Signed-off-by: Aaron Lauterer
---
[..]
diff --git a/proxmox-auto-installer/src/answer.rs
b/proxmox-auto-installer/src/answer.rs
new file mode 100644
index 000..96e5608
--- /dev/null
+++ b/proxmox-auto-installer/src/answer.rs
@@ -0,0 +1,256 @@
[..]
+#[derive(Clone, Deserialize, Debug)]
+pub struct Global {
+pub country: String,
+pub fqdn: Fqdn,
+pub keyboard: String,
+pub mailto: String,
+pub timezone: String,
+pub password: String,
+pub pre_command: Option>,
+pub post_command: Option>,
+pub reboot_on_error: Option,
How about using
#[serde(default)]
pub reboot_on_error: bool,
here? Would make checks using this a bit simpler as well.
Thanks, I'll check it and also in the other instances.
+}
+
+#[derive(Clone, Deserialize, Debug)]
+struct NetworkInAnswer {
+pub use_dhcp: Option,
Same here as for `reboot_on_error`.
+pub cidr: Option,
+pub dns: Option,
+pub gateway: Option,
+// use BTreeMap to have keys sorted
Since this comment appears multiple times in this, how about moving this
into a module-level comment, explaining it there with a full sentence?
will do
+pub filter: Option>,
+}
+
[..]
+
+#[derive(Clone, Deserialize, Debug)]
+pub struct DiskSetup {
+pub filesystem: Filesystem,
+pub disk_list: Option>,
Could this be a
#[serde(default)]
pub disk_list: Vec,
as well? Both a missing & an empty list are invalid, right?
Yes, both would be invalid.
+// use BTreeMap to have keys sorted
+pub filter: Option>,
+pub filter_match: Option,
+pub zfs: Option,
+pub lvm: Option,
+pub btrfs: Option,
+}
+
[..]
+
+impl TryFrom for Disks {
+type Error = &'static str;
+
+fn try_from(source: DiskSetup) -> Result {
+if source.disk_list.is_none() && source.filter.is_none() {
+return Err("Need either 'disk_list' or 'filter' set");
+}
+if source.disk_list.clone().is_some_and(|v| v.is_empty()) {
nit: .as_ref() works here as well and would avoid a allocation.
thx
+return Err("'disk_list' cannot be empty");
+}
+if source.disk_list.is_some() && source.filter.is_some() {
+return Err("Cannot use both, 'disk_list' and 'filter'");
+}
+
+let disk_selection = match source.disk_list {
+Some(disk_list) => DiskSelection::Selection(disk_list),
+None => DiskSelection::Filter(source.filter.unwrap()),
+};
+
+// TODO: improve checks for foreign FS options. E.g. less verbose and
handling new FS types
+// automatically
+let fs_options;
+let fs = match source.filesystem {
This could be a
let (fs, fs_options) = match source.filesystem {
..
+Filesystem::Xfs => {
+if source.zfs.is_some() || source.btrfs.is_some() {
+return Err("make sure only 'lvm' options are set");
+}
+fs_options =
FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
+FsType::Xfs
.. and then here instead
(FsType::Xfs, FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default(
as well for all the below cases, of course.
thx
+}
+Filesystem::Ext4 => {
+if source.zfs.is_some() || source.btrfs.is_some() {
+return Err("make sure only 'lvm' options are set");
+}
+fs_options =
FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
+FsType::Ext4
+}
+Filesystem::Zfs => {
+if source.lvm.is_some() || source.btrfs.is_some() {
+return Err("make sure only 'zfs' options are set");
+}
+if source.zfs.is_none() || source.zfs.is_some_and(|v|
v.raid.is_none()) {
+return Err("ZFS raid level 'zfs.raid' must be set");
+}
+fs_options = FsOptions::ZFS(source.zfs.unwrap());
+FsType::Zfs(source.zfs.unwrap().raid.unwrap())
+}
+Filesystem::Btrfs => {
+if source.zfs.is_some() || source.lvm.is_some() {
+return Err("make sure only 'btrfs' options are set");
+}
+if source.btrfs.is_none() || source.btrfs.is_some_and(|v|
v.raid.is_none()) {
+return Err("BRFS raid level 'btrfs.raid' must be set");
+}
+fs_options = FsOptions::BRFS(source.btrfs.unwrap());
+FsType::Btrfs(source.btrfs.unwrap().raid.unwrap())
Maybe make it a bit more succinctly like
match source.btr
Re: [pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition
Mostly just some comments regarding the struct (member) definitions, to
make them (and their accompanying) checks a bit simpler.
On Thu, Mar 28, 2024 at 02:50:07PM +0100, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer
> ---
[..]
> diff --git a/proxmox-auto-installer/src/answer.rs
> b/proxmox-auto-installer/src/answer.rs
> new file mode 100644
> index 000..96e5608
> --- /dev/null
> +++ b/proxmox-auto-installer/src/answer.rs
> @@ -0,0 +1,256 @@
[..]
> +#[derive(Clone, Deserialize, Debug)]
> +pub struct Global {
> +pub country: String,
> +pub fqdn: Fqdn,
> +pub keyboard: String,
> +pub mailto: String,
> +pub timezone: String,
> +pub password: String,
> +pub pre_command: Option>,
> +pub post_command: Option>,
> +pub reboot_on_error: Option,
How about using
#[serde(default)]
pub reboot_on_error: bool,
here? Would make checks using this a bit simpler as well.
> +}
> +
> +#[derive(Clone, Deserialize, Debug)]
> +struct NetworkInAnswer {
> +pub use_dhcp: Option,
Same here as for `reboot_on_error`.
> +pub cidr: Option,
> +pub dns: Option,
> +pub gateway: Option,
> +// use BTreeMap to have keys sorted
Since this comment appears multiple times in this, how about moving this
into a module-level comment, explaining it there with a full sentence?
> +pub filter: Option>,
> +}
> +
[..]
> +
> +#[derive(Clone, Debug)]
> +pub enum NetworkSettings {
> +Dhcp(bool),
`Dhcp` as variant without the `bool` should work too. At least nothing
seems to exlicitly match on this variant from a quick grep.
> +Manual(NetworkManual),
> +}
> +
> +#[derive(Clone, Debug)]
> +pub struct NetworkManual {
> +pub cidr: CidrAddress,
> +pub dns: IpAddr,
> +pub gateway: IpAddr,
> +// use BTreeMap to have keys sorted
> +pub filter: BTreeMap,
> +}
> +
> +#[derive(Clone, Deserialize, Debug)]
> +pub struct DiskSetup {
> +pub filesystem: Filesystem,
> +pub disk_list: Option>,
Could this be a
#[serde(default)]
pub disk_list: Vec,
as well? Both a missing & an empty list are invalid, right?
> +// use BTreeMap to have keys sorted
> +pub filter: Option>,
> +pub filter_match: Option,
> +pub zfs: Option,
> +pub lvm: Option,
> +pub btrfs: Option,
> +}
> +
[..]
> +
> +impl TryFrom for Disks {
> +type Error = &'static str;
> +
> +fn try_from(source: DiskSetup) -> Result {
> +if source.disk_list.is_none() && source.filter.is_none() {
> +return Err("Need either 'disk_list' or 'filter' set");
> +}
> +if source.disk_list.clone().is_some_and(|v| v.is_empty()) {
nit: .as_ref() works here as well and would avoid a allocation.
> +return Err("'disk_list' cannot be empty");
> +}
> +if source.disk_list.is_some() && source.filter.is_some() {
> +return Err("Cannot use both, 'disk_list' and 'filter'");
> +}
> +
> +let disk_selection = match source.disk_list {
> +Some(disk_list) => DiskSelection::Selection(disk_list),
> +None => DiskSelection::Filter(source.filter.unwrap()),
> +};
> +
> +// TODO: improve checks for foreign FS options. E.g. less verbose
> and handling new FS types
> +// automatically
> +let fs_options;
> +let fs = match source.filesystem {
This could be a
let (fs, fs_options) = match source.filesystem {
..
> +Filesystem::Xfs => {
> +if source.zfs.is_some() || source.btrfs.is_some() {
> +return Err("make sure only 'lvm' options are set");
> +}
> +fs_options =
> FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
> +FsType::Xfs
.. and then here instead
(FsType::Xfs, FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default(
as well for all the below cases, of course.
> +}
> +Filesystem::Ext4 => {
> +if source.zfs.is_some() || source.btrfs.is_some() {
> +return Err("make sure only 'lvm' options are set");
> +}
> +fs_options =
> FsOptions::LVM(source.lvm.unwrap_or(LvmOptions::default()));
> +FsType::Ext4
> +}
> +Filesystem::Zfs => {
> +if source.lvm.is_some() || source.btrfs.is_some() {
> +return Err("make sure only 'zfs' options are set");
> +}
> +if source.zfs.is_none() || source.zfs.is_some_and(|v|
> v.raid.is_none()) {
> +return Err("ZFS raid level 'zfs.raid' must be set");
> +}
> +fs_options = FsOptions::ZFS(source.zfs.unwrap());
> +FsType::Zfs(source.zfs.unwrap().raid.unwrap())
> +}
> +Filesystem::Btrfs => {
> +if source.zfs.is_some() || source.lvm.is_some() {
> +
[pve-devel] [PATCH v3 09/30] auto-installer: add answer file definition
Signed-off-by: Aaron Lauterer
---
proxmox-auto-installer/Cargo.toml| 1 +
proxmox-auto-installer/src/answer.rs | 256 +++
proxmox-auto-installer/src/lib.rs| 1 +
3 files changed, 258 insertions(+)
create mode 100644 proxmox-auto-installer/src/answer.rs
diff --git a/proxmox-auto-installer/Cargo.toml
b/proxmox-auto-installer/Cargo.toml
index 67218dd..80de4fa 100644
--- a/proxmox-auto-installer/Cargo.toml
+++ b/proxmox-auto-installer/Cargo.toml
@@ -12,3 +12,4 @@ proxmox-installer-common = { path =
"../proxmox-installer-common" }
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
toml = "0.7"
+enum-iterator = "0.6.0"
diff --git a/proxmox-auto-installer/src/answer.rs
b/proxmox-auto-installer/src/answer.rs
new file mode 100644
index 000..96e5608
--- /dev/null
+++ b/proxmox-auto-installer/src/answer.rs
@@ -0,0 +1,256 @@
+use enum_iterator::IntoEnumIterator;
+use proxmox_installer_common::{
+options::{BtrfsRaidLevel, FsType, ZfsChecksumOption, ZfsCompressOption,
ZfsRaidLevel},
+utils::{CidrAddress, Fqdn},
+};
+use serde::{Deserialize, Serialize};
+use std::{collections::BTreeMap, net::IpAddr};
+
+#[derive(Clone, Deserialize, Debug)]
+#[serde(rename_all = "kebab-case")]
+pub struct Answer {
+pub global: Global,
+pub network: Network,
+#[serde(rename = "disk-setup")]
+pub disks: Disks,
+}
+
+#[derive(Clone, Deserialize, Debug)]
+pub struct Global {
+pub country: String,
+pub fqdn: Fqdn,
+pub keyboard: String,
+pub mailto: String,
+pub timezone: String,
+pub password: String,
+pub pre_command: Option>,
+pub post_command: Option>,
+pub reboot_on_error: Option,
+}
+
+#[derive(Clone, Deserialize, Debug)]
+struct NetworkInAnswer {
+pub use_dhcp: Option,
+pub cidr: Option,
+pub dns: Option,
+pub gateway: Option,
+// use BTreeMap to have keys sorted
+pub filter: Option>,
+}
+
+#[derive(Clone, Deserialize, Debug)]
+#[serde(try_from = "NetworkInAnswer")]
+pub struct Network {
+pub network_settings: NetworkSettings,
+}
+
+impl TryFrom for Network {
+type Error = &'static str;
+
+fn try_from(source: NetworkInAnswer) -> Result {
+if source.use_dhcp.is_none() || source.use_dhcp == Some(false) {
+if source.cidr.is_none() {
+return Err("Field 'cidr' must be set.");
+}
+if source.dns.is_none() {
+return Err("Field 'dns' must be set.");
+}
+if source.gateway.is_none() {
+return Err("Field 'gateway' must be set.");
+}
+if source.filter.is_none() {
+return Err("Field 'filter' must be set.");
+}
+
+Ok(Network {
+network_settings: NetworkSettings::Manual(NetworkManual {
+cidr: source.cidr.unwrap(),
+dns: source.dns.unwrap(),
+gateway: source.gateway.unwrap(),
+filter: source.filter.unwrap(),
+}),
+})
+} else {
+Ok(Network {
+network_settings: NetworkSettings::Dhcp(true),
+})
+}
+}
+}
+
+#[derive(Clone, Debug)]
+pub enum NetworkSettings {
+Dhcp(bool),
+Manual(NetworkManual),
+}
+
+#[derive(Clone, Debug)]
+pub struct NetworkManual {
+pub cidr: CidrAddress,
+pub dns: IpAddr,
+pub gateway: IpAddr,
+// use BTreeMap to have keys sorted
+pub filter: BTreeMap,
+}
+
+#[derive(Clone, Deserialize, Debug)]
+pub struct DiskSetup {
+pub filesystem: Filesystem,
+pub disk_list: Option>,
+// use BTreeMap to have keys sorted
+pub filter: Option>,
+pub filter_match: Option,
+pub zfs: Option,
+pub lvm: Option,
+pub btrfs: Option,
+}
+
+#[derive(Clone, Debug, Deserialize)]
+#[serde(try_from = "DiskSetup")]
+pub struct Disks {
+pub fs_type: FsType,
+pub disk_selection: DiskSelection,
+pub filter_match: Option,
+pub fs_options: FsOptions,
+}
+
+impl TryFrom for Disks {
+type Error = &'static str;
+
+fn try_from(source: DiskSetup) -> Result {
+if source.disk_list.is_none() && source.filter.is_none() {
+return Err("Need either 'disk_list' or 'filter' set");
+}
+if source.disk_list.clone().is_some_and(|v| v.is_empty()) {
+return Err("'disk_list' cannot be empty");
+}
+if source.disk_list.is_some() && source.filter.is_some() {
+return Err("Cannot use both, 'disk_list' and 'filter'");
+}
+
+let disk_selection = match source.disk_list {
+Some(disk_list) => DiskSelection::Selection(disk_list),
+None => DiskSelection::Filter(source.filter.unwrap()),
+};
+
+// TODO: improve checks for foreign FS options. E.g. less verbose and
handling new FS types
+// automatically
+let fs_options;
+let fs = match source.filesys
