Re: [PATCH] client: support individual repository component parameters

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 24.03.26 um 14:42 schrieb Fabian Grünbichler:
> On March 23, 2026 10:11 pm, Thomas Lamprecht wrote:
>> +These options are mutually exclusive with ``--repository``. Both forms
>> +resolve to the same internal representation, so cached login tickets and
>> +other session state are shared between them. For example, logging in with
>> +``--repository`` and then running a backup with ``--server``/``--datastore``
>> +(or vice versa) reuses the same ticket, as long as the server address and
>> +user match.
>> +
>> +The component options make it easy to change individual parts of the
>> +connection, for example switching to a different datastore or server without
>> +having to rewrite the entire repository string. They also simplify usage
>> +with API tokens, which require escaping the ``@`` separator in the compact
>> +form:
> 
> that last sentence I don't understand, the `@` is part of both a user
> and an API token authid, it's the `!` that is only there for tokens?
> 
> I've never had to escape the `@` in `--repository`..

yeah, that was more for inside perl use cases, but it's bogus; will drop.

[...]

>> @@ -70,6 +114,22 @@ Environment Variables
>>  ``PBS_REPOSITORY``
>>    The default backup repository.
>>  
>> +``PBS_SERVER``
>> +  Default backup server address. Used as a fallback when neither
>> +  ``--repository`` / ``PBS_REPOSITORY`` nor ``--server`` is given.
>> +  Requires ``PBS_DATASTORE`` to be set as well.
>> +
>> +``PBS_PORT``
>> +  Default backup server port. Defaults to ``8007`` if unset.
>> +
>> +``PBS_DATASTORE``
>> +  Default datastore name. Used as a fallback when neither ``--repository`` /
>> +  ``PBS_REPOSITORY`` nor ``--datastore`` is given.
>> +
>> +``PBS_AUTH_ID``
>> +  Default authentication identity (``user@realm`` or
>> +  ``user@realm!tokenname``). Defaults to ``root@pam`` if unset.
>> +
> 
> we also have a request open for PBS_NAMESPACE, with similar rationale as
> your changes here:
> 
> https://bugzilla.proxmox.com/show_bug.cgi?id=5340

Yeah, that makes sense to incorporate in this series, thanks for the pointer.


>> diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
>> index 7a496d14c..ad83d8cab 100644
>> --- a/pbs-client/src/tools/mod.rs
>> +++ b/pbs-client/src/tools/mod.rs
>> @@ -17,12 +17,14 @@ use proxmox_router::cli::{complete_file_name, shellword_split};

[...]

>> +/// Remove repository-related keys from a JSON Value and return the parsed [`BackupRepository`].
>> +///
>> +/// This is used by commands that forward the remaining parameters to the server API after stripping
>> +/// the repository fields.
>>  pub fn remove_repository_from_value(param: &mut Value) -> Result<BackupRepository, Error> {
>> -    if let Some(url) = param
>> +    let map = param
>>          .as_object_mut()
>> -        .ok_or_else(|| format_err!("unable to get repository (parameter is not an object)"))?
>> -        .remove("repository")
>> -    {
>> -        return url
>> -            .as_str()
>> -            .ok_or_else(|| format_err!("invalid repository value (must be a string)"))?
>> -            .parse();
>> +        .ok_or_else(|| format_err!("unable to get repository (parameter is not an object)"))?;
>> +
>> +    let to_string = |v: Value| v.as_str().map(String::from);
>> +
>> +    let args = BackupRepositoryArgs {
>> +        repository: map.remove("repository").and_then(to_string),
>> +        server: map.remove("server").and_then(to_string),
>> +        port: map
>> +            .remove("port")
>> +            .and_then(|v| v.as_u64())
>> +            .map(|p| p as u16),
>> +        datastore: map.remove("datastore").and_then(to_string),
>> +        auth_id: map
>> +            .remove("auth-id")
>> +            .and_then(to_string)
>> +            .map(|s| s.parse::<Authid>())
>> +            .transpose()?,
>> +    };
>> +
>> +    match BackupRepository::try_from(args) {
>> +        Ok(repo) => Ok(repo),
>> +        Err(_) => {
>> +            // Fall back to environment.
>> +            if let Some(url) = get_default_repository() {
>> +                return url.parse();
>> +            }
>> +            let env_args = args_from_env();
>> +            if env_args.has_atoms() {
>> +                return BackupRepository::try_from(env_args);
>> +            }
>> +            bail!("repository not passed via CLI options and unable to get (default) repository from environment");
> 
> a similar issue like below also exists here I think, the code path below
> was just how I triggered it..
> 
>> +        }
>>      }
>> -
>> -    get_default_repository()
>> -        .ok_or_else(|| format_err!("unable to get default repository"))?
>> -        .parse()
>>  }
>>  
>> +/// Extract a [`BackupRepository`] from CLI parameters.
>> +///
>> +/// Resolution precedence:
>> +/// 1. CLI `--repository` + CLI atoms → error (mutually exclusive)
> 
> this doesn't work nicely in practice atm, e.g. when I run
> 
> `proxmox-backup-client backup .. --repository "something" --auth-id "something"` :
> 
> Error: unable to get (default) repository
> 
> instead of a nice error point at the invalid combination of arguments..

ack
 
>> +/// 2. CLI atoms (at least `--datastore`)
>> +/// 3. CLI `--repository`
>> +/// 4. `PBS_REPOSITORY` environment variable / systemd credential
>> +/// 5. `PBS_SERVER`/`PBS_PORT`/`PBS_DATASTORE`/`PBS_AUTH_ID` environment variables
>>  pub fn extract_repository_from_value(param: &Value) -> Result<BackupRepository, Error> {
>> -    let repo_url = param["repository"]
>> -        .as_str()
>> -        .map(String::from)
>> -        .or_else(get_default_repository)
>> -        .ok_or_else(|| format_err!("unable to get (default) repository"))?;
>> +    let args = args_from_value(param);
>>  
>> -    let repo: BackupRepository = repo_url.parse()?;
>> -
>> -    Ok(repo)
>> +    match BackupRepository::try_from(args) {
>> +        Ok(repo) => Ok(repo),
>> +        Err(_) => {
> 
> probably this error here
> 
>> +            // Fall back to environment.
>> +            if let Some(url) = get_default_repository() {
>> +                return url.parse();
>> +            }
>> +            let env_args = args_from_env();
>> +            if env_args.has_atoms() {
>> +                return BackupRepository::try_from(env_args);
>> +            }
>> +            bail!("unable to get (default) repository");
> 
> should be printed here? or we should not fall back to the environment if
> at least one atom or the URL is given as argument? what about mix and
> match, e.g. setting the server via the env, but the datastore and
> namespace via arguments?

I checked a few CLI tools and most allow mixing environment and CLI for
different such atoms, e.g. restic or borg, but also others like mysql or
psql, and rethinking this also what I would expect to happen. I'll recheck
this and try to add some unit tests to check and encode the inteded behavior.


>> +/// Extract a [`BackupRepository`] from a parameter map (used for shell
>> +/// completion callbacks).
>>  pub fn extract_repository_from_map(param: &HashMap<String, String>) -> Option<BackupRepository> {
>> -    param
>> -        .get("repository")
>> -        .map(String::from)
>> -        .or_else(get_default_repository)
>> -        .and_then(|repo_url| repo_url.parse::<BackupRepository>().ok())
>> +    let args = BackupRepositoryArgs {
>> +        repository: param.get("repository").cloned(),
>> +        server: param.get("server").cloned(),
>> +        port: param.get("port").and_then(|p| p.parse().ok()),
>> +        datastore: param.get("datastore").cloned(),
>> +        auth_id: param.get("auth-id").and_then(|s| s.parse().ok()),
>> +    };
>> +
>> +    if let Ok(repo) = BackupRepository::try_from(args) {
>> +        return Some(repo);
>> +    }
>> +
>> +    // Fall back to environment: compound URL, then atoms.
>> +    if let Some(url) = get_default_repository() {
>> +        if let Ok(repo) = url.parse() {
>> +            return Some(repo);
>> +        }
>> +    }
>> +
>> +    let env_args = args_from_env();
>> +    if env_args.has_atoms() {
>> +        return BackupRepository::try_from(env_args).ok();
>> +    }
>> +
>> +    None
>>  }
> 
> this helper probably requires similar attention for consistency's sake
> 


Will recheck more closely, thanks for taking a look!