Re: [pdm-devel] [PATCH datacenter-manager v2 13/14] ui: auto-installer: add prepared answer configuration panel

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

Thanks for the review!

(Trimming messages would be greatly appreciated btw, makes finding the
inline comments a lot easier!)

On Tue Dec 9, 2025 at 2:01 PM CET, Lukas Wagner wrote:
[..]
>> +pub async fn submit(
>> +    url: &str,
>> +    existing_id: Option<&str>,
>> +    mut config: serde_json::Value,
>> +) -> Result<()> {
>> +    let obj = config.as_object_mut().expect("always an object");
>> +
>> +    let fs_opts = collect_fs_options_into_propstring(obj);
>> +    obj.insert("filesystem-options".to_owned(), json!(fs_opts));
>> +
>> +    let root_ssh_keys = collect_lines_into_array(obj.remove("root-ssh-keys"));
>> +    let target_filter = collect_lines_into_array(obj.remove("target-filter"));
>> +    let disk_filter = collect_lines_into_array(obj.remove("disk-filter-text"));
>> +    let netdev_filter = collect_lines_into_array(obj.remove("netdev-filter-text"));
>> +
>> +    config["root-ssh-keys"] = root_ssh_keys;
>> +    config["target-filter"] = target_filter;
>> +    config["disk-filter"] = disk_filter;
>> +    config["netdev-filter"] = netdev_filter;
>> +
>> +    if let Some(id) = existing_id {
>> +        config["id"] = json!(id);
>> +        let data = delete_empty_values(
>> +            &config,
>> +            &[
>> +                "root-ssh-keys",
>> +                "post-hook-base-url",
>> +                "post-hook-cert-fp",
>> +                "disk-filter",
>> +                "netdev-filter",
>> +            ],
>> +            true,
>> +        );
>> +        proxmox_yew_comp::http_put(url, Some(data)).await
>> +    } else {
>> +        proxmox_yew_comp::http_post(url, Some(config)).await
>> +    }
>> +}
>
> In general, I think it would be better to add bindings for these new
> APIs in the pdm_client crate and then use the actual PDM client here, as
> we do in most other places in the GUI.

Should the PdmClient method then just receive as `serde_json::Value` as
we already have here? Doing an entire round-trip through serde just for
having type-safety (although nice, of course) at this bit seems rather a
bit excessive.

[..]
>> +        .with_field(
>> +            tr!("PDM API base URL"),
>> +            Field::new()
>> +                .name("post-hook-base-url")
>> +                .tip(tr!(
>> +                    "Base URL this PDM instance is reachable from the target host"
>> +                ))
>> +                .value(
>> +                    config
>> +                        .post_hook_base_url
>> +                        .clone()
>> +                        .or_else(|| pdm_origin().map(|s| format!("{s}/api2"))),
>
> I think this is lacking a `/json` at the end - anyways, the /api2/json
> part could probably be added automatically, right? So that the user just
> needs to provide the base URL? (e.g. https://somehost:8443)

Yeah, probably. TBH I wasn't if we want to support different
base-/sub-paths for the API, but guess that's rather out-of-scope then.

>> diff --git a/ui/src/auto_installer/edit_window.rs b/ui/src/auto_installer/edit_window.rs
>> new file mode 100644
>> index 0000000..7054eea
>> --- /dev/null
>> +++ b/ui/src/auto_installer/edit_window.rs
>> @@ -0,0 +1,105 @@
>> +//! Implements the configuration dialog UI for the auto-installer integration.
>> +
[..]
>> +pub struct EditAnswerWindowComponent {}
>
> This also could be private, I think.

Ack, some for all the other instances.

[..]
>> +async fn delete_entry(key: Key) -> Result<()> {
>> +    let url = format!(
>> +        "/auto-install/prepared/{}",
>> +        percent_encode_component(&key.to_string())
>> +    );
>> +    proxmox_yew_comp::http_delete(&url, None).await
>> +}
>
> Here as well, I think it would be nicer to implement this in the
> pdm_client crate.

Will do.


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