Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

From: Christian Ebner <c.ebner@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Gabriel Goller <g.goller@proxmox.com>
Subject: Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group
Date: Wed, 17 Apr 2024 16:26:21 +0200	[thread overview]
Message-ID: <108e3484-c76c-4ff9-8160-c15faea41ee7@proxmox.com> (raw)
In-Reply-To: <fdae22a8-7f9d-4a4e-a812-e06711f39ac5@proxmox.com>

On 4/17/24 16:15, Christian Ebner wrote:
> Hi,
> thanks for tackling this issue.
> 
> The group forget command is something I missed quite a lot when doing 
> testing on PBS with accumulating snapshots in a group an not wanting to 
> fallback to the UI (I actually always went the route of deleting the 
> snapshot folder).
> 
> Tested the following:
> - Created a few backup snapshots, creating thereby a new backup group
> - Tried to delete the group while backup is running
>    result was as expected: delete failed, missing lock error
> - Tried to delete the group while a restore is running
>    result was as expected: delete failed, missing lock (although 
> different from the previous one)
> - Tried to remove without any other task running
>    result was as expected: group deleted, including the group folder in 
> the datastore
> - Tried to delete empty group
>    result was as expected: group deleted, including the group folder in 
> the datastore
> 
> Two further things I noticed:
> 
> The confirmation dialog also allows me to type in `yolo` or any string 
> starting with `yY` and nevertheless accepts this as valid confirmation 
> input (similar for negation). Should we limit this further?
> 
> When one tries to delete a non existing group, the dialog asks me for 
> confirmation, failing however afterwards with an error message, leaking 
> also the datastore path to the client. While the former is not an issue 
> and the intention is to be able to remove empty groups, the latter is 
> not okay in my opinion.
> So either check if the group even exists before asking for confirmation, 
> or map the error to not leak the datastore path.

Forgot to add that the same applies for the datastore locking, the 
client should not need to know the paths there either, this should 
concern the server only.

> 
> For the rest consider this:
> 
> Tested-by: Christian Ebner <c.ebner@proxmox.com>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel