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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 04.12.20 12:30, Dominik Csapak wrote:
> 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)
> 

I mean yes but no. It's ordered delete first, so it actually just behaves like
a set (update).