Re: [PATCH v4 5/5] fix #5340: client: repository: add PBS_NAMESPACE environment variable

[Christian Ebner <c.ebner@proxmox.com>]

2 nits inline, might however be folder in when appying

On 4/10/26 5:43 PM, Thomas Lamprecht wrote:
> Add PBS_NAMESPACE as a fallback when --ns is not given on the CLI,
> addressing the request in bug #5340.
> 
> The env var is supported uniformly across all client tools
> (proxmox-backup-client, proxmox-file-restore, proxmox-backup-debug).
> Like the other PBS_* atom env vars, it provides a default that can be
> overridden by the corresponding CLI option.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
> 
> changes v3 -> v4:
> - env var fallback now lives in the shared optional_ns_param() in
>    pbs-client tools (moved in patch 3), so this patch only upgrades that
>    single function instead of touching all three binaries
> - made ENV_VAR_PBS_NAMESPACE private (consistent with other
>    ENV_VAR_PBS_* constants, no external users after the move)
> 
>   docs/backup-client.rst      |  3 +++
>   pbs-client/src/tools/mod.rs | 16 +++++++++++-----
>   2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/backup-client.rst b/docs/backup-client.rst
> index 1d464a007..e2a05d4fc 100644
> --- a/docs/backup-client.rst
> +++ b/docs/backup-client.rst
> @@ -131,6 +131,9 @@ Environment Variables
>     Authentication identity (``user@realm`` or ``user@realm!tokenname``).
>     Defaults to ``root@pam`` if unset.
>   
> +``PBS_NAMESPACE``
> +  Backup namespace. Used as a fallback when ``--ns`` is not given.
> +
>   ``PBS_PASSWORD``
>     When set, this value is used as the password for the backup server.
>     You can also set this to an API token secret.
> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
> index fa708a8b5..e034b9a31 100644
> --- a/pbs-client/src/tools/mod.rs
> +++ b/pbs-client/src/tools/mod.rs
> @@ -36,6 +36,7 @@ const ENV_VAR_PBS_SERVER: &str = "PBS_SERVER";
>   const ENV_VAR_PBS_PORT: &str = "PBS_PORT";
>   const ENV_VAR_PBS_DATASTORE: &str = "PBS_DATASTORE";
>   const ENV_VAR_PBS_AUTH_ID: &str = "PBS_AUTH_ID";
> +const ENV_VAR_PBS_NAMESPACE: &str = "PBS_NAMESPACE";
>   
>   /// Directory with system [credential]s. See systemd-creds(1).
>   ///
> @@ -339,13 +340,17 @@ pub fn extract_repository_from_map(param: &HashMap<String, String>) -> Option<Ba
>       resolve_repository(cli).ok()
>   }
>   
> -/// Extract a [`BackupNamespace`] from CLI parameters.
> +/// Extract a [`BackupNamespace`] from CLI parameters, falling back to PBS_NAMESPACE.
>   pub fn optional_ns_param(param: &Value) -> Result<BackupNamespace, Error> {
> -    Ok(match param.get("ns") {
> -        Some(Value::String(ns)) => ns.parse()?,
> +    match param.get("ns") {
> +        Some(Value::String(ns)) => return ns.parse().map_err(Error::from),

nit: introduces a clippy useless_conversion warning, the error type here 
already is anyhow::Error

>           Some(_) => bail!("invalid namespace parameter"),
> -        None => BackupNamespace::root(),
> -    })
> +        None => {}
> +    }
> +    if let Ok(ns) = std::env::var(ENV_VAR_PBS_NAMESPACE) {
> +        return ns.parse().map_err(Error::from);


nit: same as above

> +    }
> +    Ok(BackupNamespace::root())
>   }
>   
>   pub fn connect(repo: &BackupRepository) -> Result<HttpClient, Error> {
> @@ -849,6 +854,7 @@ mod tests {
>           ENV_VAR_PBS_PORT,
>           ENV_VAR_PBS_DATASTORE,
>           ENV_VAR_PBS_AUTH_ID,
> +        ENV_VAR_PBS_NAMESPACE,
>           ENV_VAR_CREDENTIALS_DIRECTORY,
>       ];
>