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

["Lukas Wagner" <l.wagner@proxmox.com>]

On Tue Dec 16, 2025 at 3:28 PM CET, Christoph Heiss wrote:
> Thanks for the review!
>
> (Trimming messages would be greatly appreciated btw, makes finding the
> inline comments a lot easier!)

Ack!

>
> 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.

I think using the actual Rust types would be nice, considering that
pdm-client could have other consumers than the UI at some point. I think
we do the same thing in a couple other places as well.

>
> [..]
>>> +        .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.
>

Fair point! I think supporting different sub-paths would be a way larger
endeavor than this pre-filled URL here; and I don't think think that
this is something that we are going to add support for. Therefore I
don't think this should be a reason to worsen the UX here in
anticipation :)




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