From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C35311FF13C for ; Thu, 02 Apr 2026 10:54:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 838C5F9FF; Thu, 2 Apr 2026 10:54:48 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Subject: Re: [PATCH v3 3/5] client: migrate commands to flattened repository args From: Wolfgang Bumiller To: Thomas Lamprecht In-Reply-To: <20260401225305.4069441-4-t.lamprecht@proxmox.com> References: <20260401225305.4069441-1-t.lamprecht@proxmox.com> <20260401225305.4069441-4-t.lamprecht@proxmox.com> Date: Thu, 02 Apr 2026 10:54:08 +0200 Message-Id: <177512004836.24063.6839896113631114592.b4-review@b4> X-Mailer: b4 0.15.1 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775119992104 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.413 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: YNNQJ4JJWEEJGFVN4YB2UZZFIEHGKUIK X-Message-ID-Hash: YNNQJ4JJWEEJGFVN4YB2UZZFIEHGKUIK X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 02 Apr 2026 00:48:59 +0200, Thomas Lamprecht 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.", &[ , &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(¶m)?; > - 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(¶m)?; > - 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, > - ns: Option, > param: Value, > ) -> Result<(), Error> { > let repo = extract_repository_from_value(¶m)?; > > 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) --