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

Thomas Lamprecht
2026-03-25T11:27:56Z

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!