Re: [pve-devel] [PATCH manager 1/7] api: cluster/metricserver: prevent simultaneosly setting and deleting of property

[Dominik Csapak <d.csapak@proxmox.com>]

On 12/3/20 10:05 AM, Thomas Lamprecht wrote:
> On 02.12.20 10:21, Dominik Csapak wrote:
>> like we do in other apis of section configs (e.g. storage)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Cluster/MetricServer.pm | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/PVE/API2/Cluster/MetricServer.pm b/PVE/API2/Cluster/MetricServer.pm
>> index 9a14985e..ec3c7b75 100644
>> --- a/PVE/API2/Cluster/MetricServer.pm
>> +++ b/PVE/API2/Cluster/MetricServer.pm
>> @@ -213,6 +213,8 @@ __PACKAGE__->register_method ({
>>   		    my $d = $options->{$k} || die "no such option '$k'\n";
>>   		    die "unable to delete required option '$k'\n" if !$d->{optional};
>>   		    die "unable to delete fixed option '$k'\n" if $d->{fixed};
>> +		    die "cannot set and delete property '$k' at the same time!\n"
>> +			if defined($opts->{$k});
>>   
>>   		    delete $data->{$k};
>>   		}
>>
> 
> That counts as API change, strictly speaking..

ok, so should we leave it as is for now?

> For container and VMs we order
> deletions before setting the value, and the one from container is the last
> one which got some actual thoughts and discussion going on, IIRC, albeit not
> to sure if about that exact behavior (as it was probably pre-existing).
> 
> It'd be good to at least decide for one behavior and try making that universal,
> as else this is confusing..
> 

yeah that makes sense (though i think the ordering is irrelevant, since
even in container you cannot set and delete at the same time)