Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Fiona Ebner <f.ebner@proxmox.com>]

Am 08.08.23 um 11:13 schrieb Philipp Hufnagl:
> When a member of 2 pools wants to transfer a
>  vm/container to an other pool they can not do that. The vv/container would
>  have first to be removed form the current pool resulting in a loss of
>  privileges of the pool member for this vm/contianer. This feature introduces
>  a way to transfer a vm between pools, guarded by a checkbox from accidental
>  transfers
> 

Nit: there's an additional space at the start of each line, also some
typos, e.g. vv/contianer

> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  PVE/API2/Pool.pm                 | 19 +++++++++++++++++--
>  www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
>  2 files changed, 31 insertions(+), 5 deletions(-)

The backend and UI changes should be two different patches.

> 
> diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
> index 007fc815..2f69911a 100644
> --- a/PVE/API2/Pool.pm
> +++ b/PVE/API2/Pool.pm
> @@ -131,6 +131,11 @@ __PACKAGE__->register_method ({
>  		type => 'string',  format => 'pve-storage-id-list',
>  		optional => 1,
>  	    },
> +	    transfer => {
> +		description => "Allow transfering vms to another pool.",

s/transfering vms/transferring VMs/

> +		type => 'boolean',
> +		optional => 1,
> +	    },
>  	    delete => {
>  		description => "Remove vms/storage (instead of adding it).",
>  		type => 'boolean',
> @@ -165,8 +170,18 @@ __PACKAGE__->register_method ({
>  		    } else {
>  			die "VM $vmid is already a pool member\n" if $pool_config->{vms}->{$vmid};
>  			my $existing_pool = $usercfg->{vms}->{$vmid};
> -			die "VM $vmid belongs already to pool '$existing_pool'\n" if defined($existing_pool);
> -
> +			if(defined($existing_pool) )
> +			{
> +			    if($param->{transfer})

Style nit: both ifs are missing a space after the keyword, the upper one
has a space too much before the parenthesis. The curly braces should be
on the same line

> +			    {
> +				my $existing_pool_config = $usercfg->{pools}->{$existing_pool};

The permission for the original pool should be checked here?! Or is that
already done somewhere?

> +				delete $existing_pool_config->{vms}->{$vmid};
> +			    }
> +			    else
> +			    {

Style nit: should be } else {

> +				die "VM $vmid belongs already to pool '$existing_pool' and transfer is not set\n";
> +			    }
> +			}
>  			$pool_config->{vms}->{$vmid} = 1;
>  			$usercfg->{vms}->{$vmid} = $pool;
>  		    }