On Tue Sep 30, 2025 at 4:58 PM CEST, Michael Köppl wrote: > This patch series aims to fix #6613 [0]. Although an implementation was > proposed in the past, it was not applied since it was unclear how to > handle the case where the removed resource is the last resource in a > rule. > [...] > > Changes since v2: > - Rebase on latest master > - Split up widget-toolkit patches to make review easier. The intention > for patches 1/4 to 3/4 of widget-toolkit is to keep the functionality > of SafeDestroy intact (no changes in behavior for SafeDestroyGuest, > SafeDestroyStorage, and other SafeDestroy dialogs), only adding > additional functionality that is then moved to ConfirmRemoveDialog in > 4/4, where the default settings are then changed in > ConfirmRemoveDialog and SafeDestroy explicitly sets the parameters to > keep behavior intact. > - Instead of replacing SafeDestroy with ConfirmRemoveDialog and > requiring additional changes to windows extending SafeDestroy, make > SafeDestroy extend ConfirmRemoveDialog, encapsulating the specific > settings of a SafeDestroy dialog. This also makes the original > follow-up series obsolete [2].
The changes look good to me and it's much easier to review now that the changes done to SafeDestroy are visible as a diff instead.. It might be considered as confusing that first functionality is added to SafeDestroy and afterwards moved to another new component after only a few patches, but I don't see that as a reason to not ACK this. I tested this for all cases for the CLI/API/GUI for removing a HA resource and the whole VM/CT with purge set/cleared and it works as expected. I also tested that other SafeDestroy users still work for PVE and PBS (only installing proxmox-widget-toolkit there) and that worked fine as well. Thanks for the work, consider this as: Reviewed-by: Daniel Kral <[email protected]> Tested-by: Daniel Kral <[email protected]> _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
