Re: [PATCH v3 3/5] client: migrate commands to flattened repository args

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, 02 Apr 2026 00:48:59 +0200, Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> Replace the individual repository and ns property definitions in all
> client command API schema macros with the flattened repository args
> struct. This makes --server, --port, --datastore, --auth-id, and --ns
> available on every command that previously only accepted --repository.
> 
> For some commands the --ns namespace option doesn't make really sense,
> like e.g. login, but that is really not different from the
> pre-existing datastore component that one could pass through the
> repository URL.

Then again we could just use `BackupRepositoryArgs` in those instead of
the full `BackupTargetArgs`?

On the other hand - and we probably already discussed this somewhere -
many (if not all) of our commands call `record_repository` to "remember"
the repository which, as far I can see, we mostly use for command
completion, but we could also consider an option to just use the last
recorded repository as another fallback for when no CLI options or env
vars were set. Then, having `--ns` on `login` would very much make
sense.

Then again, I suppose for the purpose of completion, it still might make
sense now... (except for maybe the `logout` command 🤷

>
>
> diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
> index e815c8a9c..91e85dde5 100644
> --- a/proxmox-backup-client/src/mount.rs
> +++ b/proxmox-backup-client/src/mount.rs
> @@ -53,7 +53,12 @@ const API_METHOD_MOUNT: ApiMethod = ApiMethod::new(
>                  false,
>                  &StringSchema::new("Target directory path.").schema()
>              ),
> +            ("ns", true, &BackupNamespace::API_SCHEMA),
>              ("repository", true, &REPO_URL_SCHEMA),
> +            ("server", true, &BACKUP_REPO_SERVER_SCHEMA),
> +            ("port", true, &BACKUP_REPO_PORT_SCHEMA),
> +            ("datastore", true, &DATASTORE_SCHEMA),
> +            ("auth-id", true, &Authid::API_SCHEMA),

While this is probably fine, since we only have 2 such raw instances,
this *could* also instead switch from `ApiMethod::new` to
`ApiMethod::new_full`, wrap the `&ObjectSchema` inside a

    ParameterSchema::AllOf(&AllOfSchema::new(
        "Mount pxar archive.",
        &[
            <The object schema here with a .schema() added at the end>,
            &BackupRepositoryArgs::API_SCHEMA,
        ],
    ))

to avoid copying the contents.

>
> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
> index bf6cf9aed..0de56a7d6 100644
> --- a/proxmox-file-restore/src/main.rs
> +++ b/proxmox-file-restore/src/main.rs
> @@ -280,7 +274,11 @@ async fn list(
>      param: Value,
>  ) -> Result<(), Error> {
>      let repo = extract_repository_from_value(&param)?;
> -    let ns = ns.unwrap_or_default();
> +    let ns: BackupNamespace = param["ns"]
> +        .as_str()
> +        .map(|s| s.parse())
> +        .transpose()?
> +        .unwrap_or_default();

Should we move `optional_ns_param()` from the client to `pbs-client`'s
`tools`? Since we this twice here and once in the `-debug` binary as
well.

Patch 5 would then only need to adapt the 1 function instead of all of
these instances.

> @@ -436,7 +429,11 @@ async fn extract(
>      param: Value,
>  ) -> Result<(), Error> {
>      let repo = extract_repository_from_value(&param)?;
> -    let namespace = ns.unwrap_or_default();
> +    let namespace: BackupNamespace = param["ns"]
> +        .as_str()
> +        .map(|s| s.parse())
> +        .transpose()?
> +        .unwrap_or_default();

(since this is the same (and extended by patch 5))

>
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index 116216e51..88b4199f7 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -108,13 +103,16 @@ async fn diff_archive_cmd(
>      archive_name: BackupArchiveName,
>      compare_content: bool,
>      color: Option<ColorMode>,
> -    ns: Option<BackupNamespace>,
>      param: Value,
>  ) -> Result<(), Error> {
>      let repo = extract_repository_from_value(&param)?;
>  
>      let color = color.unwrap_or_default();
> -    let namespace = ns.unwrap_or_else(BackupNamespace::root);
> +    let namespace: BackupNamespace = param["ns"]
> +        .as_str()
> +        .map(|s| s.parse())
> +        .transpose()?
> +        .unwrap_or_default();

(and this, too)

--