[pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Philipp Hufnagl]

When a snapshot gets deleted (forgotten), the proxmox backup client
currently returns returns
"Result: {
  "data": null
}"

This feedback may confuse users therefore this patch changes the output
to

"Successfully deleted snapshot <snapshot>"

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 proxmox-backup-client/src/snapshot.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
index 4ca8015f..1d985a2e 100644
--- a/proxmox-backup-client/src/snapshot.rs
+++ b/proxmox-backup-client/src/snapshot.rs
@@ -188,13 +188,14 @@ async fn forget_snapshots(param: Value) -> Result<Value, Error> {
 
     let path = format!("api2/json/admin/datastore/{}/snapshots", repo.store());
 
-    let result = client
+    client
         .delete(&path, Some(snapshot_args(&backup_ns, &snapshot)?))
         .await?;
 
     record_repository(&repo);
 
-    Ok(result)
+    println!("Successfully deleted snapshot {}", snapshot);
+    Ok(Value::Null)
 }
 
 #[api(
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Dominik Csapak]

LGTM

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 10/3/23 16:07, Philipp Hufnagl wrote:
> When a snapshot gets deleted (forgotten), the proxmox backup client
> currently returns returns
> "Result: {
>    "data": null
> }"
> 
> This feedback may confuse users therefore this patch changes the output
> to
> 
> "Successfully deleted snapshot <snapshot>"
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>   proxmox-backup-client/src/snapshot.rs | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
> index 4ca8015f..1d985a2e 100644
> --- a/proxmox-backup-client/src/snapshot.rs
> +++ b/proxmox-backup-client/src/snapshot.rs
> @@ -188,13 +188,14 @@ async fn forget_snapshots(param: Value) -> Result<Value, Error> {
>   
>       let path = format!("api2/json/admin/datastore/{}/snapshots", repo.store());
>   
> -    let result = client
> +    client
>           .delete(&path, Some(snapshot_args(&backup_ns, &snapshot)?))
>           .await?;
>   
>       record_repository(&repo);
>   
> -    Ok(result)
> +    println!("Successfully deleted snapshot {}", snapshot);
> +    Ok(Value::Null)
>   }
>   
>   #[api(

Re: [pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Thomas Lamprecht]

Am 03/10/2023 um 16:07 schrieb Philipp Hufnagl:
> -    Ok(result)
> +    println!("Successfully deleted snapshot {}", snapshot);

please use inline variables wherever easily possible for new code:

println!("Successfully deleted snapshot {snapshot}");

but I'm not sure if we actually want to print anything here, did you
check what the other "action" (i.e., non-GET ones) do, If they all,
or at least most of them, print such reports too it can be fine, but
otherwise this would add inconsistency and simply doing nothing (i.e.,
exiting with SUCCESS) would be enough.

Re: [pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Philipp Hufnagl]

On 10/13/23 13:30, Thomas Lamprecht wrote:
> Am 03/10/2023 um 16:07 schrieb Philipp Hufnagl:
>> -    Ok(result)
>> +    println!("Successfully deleted snapshot {}", snapshot);
> 
> please use inline variables wherever easily possible for new code:
> 
> println!("Successfully deleted snapshot {snapshot}");
> 
> but I'm not sure if we actually want to print anything here, did you
> check what the other "action" (i.e., non-GET ones) do, If they all,
> or at least most of them, print such reports too it can be fine, but
> otherwise this would add inconsistency and simply doing nothing (i.e.,
> exiting with SUCCESS) would be enough.
> 

You are right! For example 'proxmox-backup-client snapshot notes
update' does not produce any output. Ill make a v 2 on Monday removing
the output!

Re: [pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Thomas Lamprecht]

Am 13/10/2023 um 16:57 schrieb Philipp Hufnagl:
> On 10/13/23 13:30, Thomas Lamprecht wrote:
>> but I'm not sure if we actually want to print anything here, did you
>> check what the other "action" (i.e., non-GET ones) do, If they all,
>> or at least most of them, print such reports too it can be fine, but
>> otherwise this would add inconsistency and simply doing nothing (i.e.,
>> exiting with SUCCESS) would be enough.
>>
> You are right! For example 'proxmox-backup-client snapshot notes
> update' does not produce any output. Ill make a v 2 on Monday removing
> the output!
> 


FWIW, I do not have anything about outputting such things, especially
if other devs/users thinks that's fine, or even desired too.

But, if we do more such things then a -q/--quiet flag could be nice, e.g.,
to avoid stray output in scripts using those commands.

That said, we can always add such output later after you fixed the weird
Result { null } print, let's keep the scope small for now.

Re: [pbs-devel] [PATCH proxmox-backup] fix #4971: client: Improve output on successful snapshot deletion

[Philipp Hufnagl]

On 10/13/23 18:29, Thomas Lamprecht wrote:
> Am 13/10/2023 um 16:57 schrieb Philipp Hufnagl:
>> On 10/13/23 13:30, Thomas Lamprecht wrote:
>>> but I'm not sure if we actually want to print anything here, did you
>>> check what the other "action" (i.e., non-GET ones) do, If they all,
>>> or at least most of them, print such reports too it can be fine, but
>>> otherwise this would add inconsistency and simply doing nothing (i.e.,
>>> exiting with SUCCESS) would be enough.
>>>
>> You are right! For example 'proxmox-backup-client snapshot notes
>> update' does not produce any output. Ill make a v 2 on Monday removing
>> the output!
>>
> 
> 
> FWIW, I do not have anything about outputting such things, especially
> if other devs/users thinks that's fine, or even desired too.
> 
> But, if we do more such things then a -q/--quiet flag could be nice, e.g.,
> to avoid stray output in scripts using those commands.
> 
> That said, we can always add such output later after you fixed the weird
> Result { null } print, let's keep the scope small for now.

I think you are right here. Since other commands do not output
anything, this command should do that as well for consistency. I think
I should remove the output and we should consider a -v/--verbose flag
in the future.