Re: [pdm-devel] [PATCH proxmox v2 05/14] installer-types: add common types used by the installer

["Christoph Heiss" <c.heiss@proxmox.com>]

Thanks for the review!

On Tue Dec 9, 2025 at 10:35 AM CET, Lukas Wagner wrote:
> Some notes inline.
>
> On Fri Dec 5, 2025 at 12:25 PM CET, Christoph Heiss wrote:
[..]
>> diff --git a/proxmox-installer-types/src/lib.rs b/proxmox-installer-types/src/lib.rs
>> new file mode 100644
>> index 00000000..07927cb0
>> --- /dev/null
>> +++ b/proxmox-installer-types/src/lib.rs
>> @@ -0,0 +1,143 @@
[..]
>> +#[derive(Copy, Clone, Eq, Deserialize, PartialEq, Serialize)]
>> +#[serde(rename_all = "lowercase")]
>> +/// Whether the system boots using legacy BIOS or (U)EFI.
>> +pub enum BootType {
>> +    /// System boots using legacy BIOS.
>> +    Bios,
>> +    /// System boots using (U)EFI.
>> +    Efi,
>> +}
>
> We already have some very similar definitions in proxmox-node-status -
> not sure how easy it would be to consolidate the two types, since the
> serialized form might not be exactly the same. Maybe the installer could
> just use custom de/serializers and reuse the types from
> proxmox-node-status?
>

Seems sensible.

Looking at it, we actually have at least two instances of this type
already:
- proxmox_node_status::types::BootMode and
- proxmox_sys::boot_mode::BootMode

The latter one would be compatible, but otherwise custom de-/serializers
are of course always an option. I will deduplicate at least this one,
not to introduce yet another one.

(There's another instance of this enum in pbs-api-types, but it's an API
type definition with an "Unknown" variant, which doesn't really fit
here.)

>> +#[allow(clippy::upper_case_acronyms)]
>> +#[derive(Debug, Clone, Copy, Deserialize, PartialEq, Serialize)]
>> +#[serde(rename_all = "lowercase")]
>> +/// The name of the product.
>> +pub enum ProxmoxProduct {
>> +    /// Proxmox Virtual Environment
>> +    PVE,
>> +    /// Proxmox Backup Server
>> +    PBS,
>> +    /// Proxmox Mail Gateway
>> +    PMG,
>> +    /// Proxmox Datacenter Manager
>> +    PDM,
>> +}
>
> I think we generally prefer regular UpperPascalCase, even for acronyms,
> as per the official Rust naming guide [1].
> So these should rather be Pve, Pbs, etc.
>
> [1] https://rust-lang.github.io/api-guidelines/naming.html

There were just moved from pve-installer, but might as well use the
chance and change it. Will rename them for v2.

>
>> +
>> +serde_plain::derive_fromstr_from_deserialize!(ProxmoxProduct);
>> +
>> +impl Display for ProxmoxProduct {
>> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> +        match self {
>> +            Self::PVE => write!(f, "Proxmox Virtualization Environment"),
>
> Virtual Environment, not Virtualization Environment :)

Good catch, thanks!

>
>> +            Self::PBS => write!(f, "Proxmox Backup Server"),
>> +            Self::PMG => write!(f, "Proxmox Mail Gateway"),
>> +            Self::PDM => write!(f, "Proxmox Datacenter Manager"),
>> +        }
>> +    }
>> +}



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