[pve-devel] [PATCH installer 0/5] allow both snake- and kebab-cased property names in the answer file

[pve-devel] [PATCH installer 0/5] allow both snake- and kebab-cased property names in the answer file

[Daniel Kral]

This is a followup to a previous discussion at [0].

Small patch series which allows both snake- and kebab-cased property
names in the configuration file for the auto installer, i.e. answer
files. This allows to introduce a migration from snake_cased towards
kebab-cased property names in the answer file to be consistent with
other configuration files, which prefer kebab-case too.

The only property key that was not changed in casing was the filter
match rules for network and block devices as those are not in our
control, but matches the udevadm output's JSON property keys (e.g.
"filter.ID_MODEL").

The last patch introduces a deprecation warning that is output to the
user when verifying answer files and preparing auto installer ISOs with
the assistant to be applied for a major version bump, i.e. PVE
9.0/Trixie-based releases as suggested by @Thomas at [0].

[0] https://lore.proxmox.com/pve-devel/0dec173a-da75-4d70-ac07-e1133c136081@proxmox.com/

Daniel Kral (5):
  auto-installer: factor out field rename casing for network config mode
  auto-installer: first-boot: allow snake- and kebabcased property names
  auto-installer: allow snake- and kebabcased property names in answer
    files
  auto-installer: add redundant kebab-case renames to config structures
  assistant: add deprecation notice for snakecased parameters

 proxmox-auto-install-assistant/src/main.rs | 24 ++++++++++++++++
 proxmox-auto-installer/src/answer.rs       | 33 +++++++++++++---------
 2 files changed, 44 insertions(+), 13 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH installer 1/5] auto-installer: factor out field rename casing for network config mode

[Daniel Kral]

No functional changes intended.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 proxmox-auto-installer/src/answer.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index c206fcc..38a048b 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -122,12 +122,10 @@ pub struct FirstBootHookInfo {
 }
 
 #[derive(Clone, Deserialize, Debug, Default, PartialEq)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 enum NetworkConfigMode {
     #[default]
-    #[serde(rename = "from-dhcp")]
     FromDhcp,
-    #[serde(rename = "from-answer")]
     FromAnswer,
 }
 
-- 
2.39.5



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

[pve-devel] [PATCH installer 2/5] auto-installer: first-boot: allow snake- and kebabcased property names

[Daniel Kral]

The default case for property names in TOML configuration files is de
facto snake_case. The answer file for the auto-installer adapted this,
but this causes inconsistencies with the property names in other Proxmox
config file formats, which are preferably in kebab-case.

The properties for the "first-boot" section were intentionally
introduced in kebab-case to steer away from snake_cased to kebab-cased
property names. Since this caused some initial confusion for users [0],
allow both cases for the time being and deprecate the snake_cased
variant of all properties in a future major version.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5973

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 proxmox-auto-installer/src/answer.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index 38a048b..e8614d2 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -118,6 +118,7 @@ pub struct FirstBootHookInfo {
     /// Retrieve the post-install script from a URL, if source == "from-url".
     pub url: Option<String>,
     /// SHA256 cert fingerprint if certificate pinning should be used, if source == "from-url".
+    #[serde(alias = "cert_fingerprint")]
     pub cert_fingerprint: Option<String>,
 }
 
-- 
2.39.5



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

[pve-devel] [PATCH installer 3/5] auto-installer: allow snake- and kebabcased property names in answer files

[Daniel Kral]

The default case for property names in TOML configuration files is de
facto snake_case. The answer file for the auto-installer adapted this,
but this causes inconsistencies with the property names in other Proxmox
config file formats, which are preferably in kebab-case.

To gradually migrate from snake_cased to kebab-cased property names in
answer files, allow both cases for the time being and deprecate the
snake_cased variant of all properties in a future major version.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 proxmox-auto-installer/src/answer.rs | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index e8614d2..c834591 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -10,6 +10,9 @@ use proxmox_installer_common::{
 use serde::{Deserialize, Serialize};
 use std::{collections::BTreeMap, io::BufRead, net::IpAddr};
 
+// NOTE New answer file properties must use kebab-case, but should allow snake_case for backwards
+// compatibility. TODO Remove the snake_cased variants in a future major version (e.g. PVE 10).
+
 // BTreeMap is used to store filters as the order of the filters will be stable, compared to
 // storing them in a HashMap
 
@@ -40,27 +43,30 @@ impl Answer {
 }
 
 #[derive(Clone, Deserialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct Global {
     pub country: String,
     pub fqdn: Fqdn,
     pub keyboard: KeyboardLayout,
     pub mailto: String,
     pub timezone: String,
+    #[serde(alias = "root_password")]
     pub root_password: Option<String>,
+    #[serde(alias = "root_password_hashed")]
     pub root_password_hashed: Option<String>,
-    #[serde(default)]
+    #[serde(alias = "reboot_on_error", default)]
     pub reboot_on_error: bool,
-    #[serde(default)]
+    #[serde(alias = "root_ssh_keys", default)]
     pub root_ssh_keys: Vec<String>,
 }
 
 #[derive(Clone, Deserialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct PostNotificationHookInfo {
     /// URL to send a POST request to
     pub url: String,
     /// SHA256 cert fingerprint if certificate pinning should be used.
+    #[serde(alias = "cert_fingerprint")]
     pub cert_fingerprint: Option<String>,
 }
 
@@ -209,12 +215,13 @@ pub struct NetworkManual {
 }
 
 #[derive(Clone, Debug, Deserialize)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct DiskSetup {
     pub filesystem: Filesystem,
-    #[serde(default)]
+    #[serde(alias = "disk_list", default)]
     pub disk_list: Vec<String>,
     pub filter: Option<BTreeMap<String, String>>,
+    #[serde(alias = "filter_match")]
     pub filter_match: Option<FilterMatch>,
     pub zfs: Option<ZfsOptions>,
     pub lvm: Option<LvmOptions>,
@@ -330,10 +337,11 @@ pub enum Filesystem {
 }
 
 #[derive(Clone, Copy, Default, Deserialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct ZfsOptions {
     pub raid: Option<ZfsRaidLevel>,
     pub ashift: Option<usize>,
+    #[serde(alias = "arc_max")]
     pub arc_max: Option<usize>,
     pub checksum: Option<ZfsChecksumOption>,
     pub compress: Option<ZfsCompressOption>,
-- 
2.39.5



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

[pve-devel] [PATCH installer 4/5] auto-installer: add redundant kebab-case renames to config structures

[Daniel Kral]

Those attributes are not necessary, but make it explicit that answer
file properties should use kebab-cased property names from now on and
therefore make it harder to accidentally add a non-kebab-cased property.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 proxmox-auto-installer/src/answer.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index c834591..5191a49 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -137,7 +137,7 @@ enum NetworkConfigMode {
 }
 
 #[derive(Clone, Deserialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 struct NetworkInAnswer {
     #[serde(default)]
     pub source: NetworkConfigMode,
@@ -350,7 +350,7 @@ pub struct ZfsOptions {
 }
 
 #[derive(Clone, Copy, Default, Deserialize, Serialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct LvmOptions {
     pub hdsize: Option<f64>,
     pub swapsize: Option<f64>,
@@ -360,7 +360,7 @@ pub struct LvmOptions {
 }
 
 #[derive(Clone, Copy, Default, Deserialize, Debug)]
-#[serde(deny_unknown_fields)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
 pub struct BtrfsOptions {
     pub hdsize: Option<f64>,
     pub raid: Option<BtrfsRaidLevel>,
-- 
2.39.5



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

[pve-devel] [PATCH for-PVE-9.0/installer 5/5] assistant: add deprecation notice for snakecased parameters

[Daniel Kral]

Print a deprecation notice if the auto-installer-assistant detects any
snakecased parameters while parsing the answer file, which is deprecated
and users should change them to kebab-cased parameters. This includes
all parameters except the filter match rules (e.g. "ID_MODEL") as those
correspond to the output of udevadm.

The detection is best effort since the TOML format allows many ways to
parse keys (e.g. '=' are allowed in quoted keys, whitespaces is allowed
around the seperating dot '.'), but should cover the relevant cases used
by our users.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 proxmox-auto-install-assistant/src/main.rs | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 969922c..c6e4249 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -584,6 +584,20 @@ fn get_udev_properties(path: impl AsRef<Path> + fmt::Debug) -> Result<String> {
     Ok(String::from_utf8(udev_output.stdout)?)
 }
 
+fn get_deprecated_properties(s: &str) -> Vec<&str> {
+    s.lines()
+        .map(|line| line.trim())
+        .filter(|line| line.contains('='))
+        // Ignore commented out config lines
+        .filter(|line| !line.starts_with('#'))
+        .filter_map(|line| line.split_once('='))
+        .filter(|(key, _)| key.contains('_'))
+        // Ignore keys for udev filter rules
+        .filter(|(key, _)| !key.starts_with("filter."))
+        .map(|(key, _)| key.trim())
+        .collect()
+}
+
 fn parse_answer(path: impl AsRef<Path> + fmt::Debug) -> Result<Answer> {
     let mut file = match fs::File::open(&path) {
         Ok(file) => file,
@@ -594,6 +608,16 @@ fn parse_answer(path: impl AsRef<Path> + fmt::Debug) -> Result<Answer> {
         bail!("Reading from file {path:?} failed: {err}");
     }
 
+    let deprecated_props = get_deprecated_properties(&contents);
+    if !deprecated_props.is_empty() {
+        let prop_list = deprecated_props.join(", ");
+        println!("The answer file contains the following snake_cased parameters: {prop_list}.");
+        println!(
+            "Snakecased parameters are deprecated in favor of kebab-cased parameters and \
+            support will be removed in the next major release.\n"
+        );
+    }
+
     match toml::from_str(&contents) {
         Ok(answer) => {
             verify_locale_settings(&answer, &serde_json::from_str(LOCALE_INFO)?)?;
-- 
2.39.5



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

[pve-devel] applied: [PATCH installer 0/5] allow both snake- and kebab-cased property names in the answer file

[Thomas Lamprecht]

Am 17.02.25 um 13:17 schrieb Daniel Kral:
> This is a followup to a previous discussion at [0].
> 
> Small patch series which allows both snake- and kebab-cased property
> names in the configuration file for the auto installer, i.e. answer
> files. This allows to introduce a migration from snake_cased towards
> kebab-cased property names in the answer file to be consistent with
> other configuration files, which prefer kebab-case too.
> 
> The only property key that was not changed in casing was the filter
> match rules for network and block devices as those are not in our
> control, but matches the udevadm output's JSON property keys (e.g.
> "filter.ID_MODEL").
> 
> The last patch introduces a deprecation warning that is output to the
> user when verifying answer files and preparing auto installer ISOs with
> the assistant to be applied for a major version bump, i.e. PVE
> 9.0/Trixie-based releases as suggested by @Thomas at [0].
> 
> [0] https://lore.proxmox.com/pve-devel/0dec173a-da75-4d70-ac07-e1133c136081@proxmox.com/
> 
> Daniel Kral (5):
>   auto-installer: factor out field rename casing for network config mode
>   auto-installer: first-boot: allow snake- and kebabcased property names
>   auto-installer: allow snake- and kebabcased property names in answer
>     files
>   auto-installer: add redundant kebab-case renames to config structures
>   assistant: add deprecation notice for snakecased parameters
> 
>  proxmox-auto-install-assistant/src/main.rs | 24 ++++++++++++++++
>  proxmox-auto-installer/src/answer.rs       | 33 +++++++++++++---------
>  2 files changed, 44 insertions(+), 13 deletions(-)
> 


applied series, thanks!

But shortly discussed off-list I squashed the commits into a single one.
As while I get how you separate it, and I'm generally in favor of not
squashing every that looks similar into the same commit, this here is
basically a single semantic change where one has a better git log "reading
flow" if one sees the full change all in a single commit.

Just for the sake of completeness: If in doubt, I'd almost always favor
having the commits split over squashed, as squashing is normally a very
quick operation; so it was IMO definitively also an OK call to send this
series the way you did.


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