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

[Thomas Lamprecht]

On 04.12.20 12:57, Thomas Lamprecht wrote:
> 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 at 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).
> 

I only looked at the internal update method and missed the check which errors 
also for CTs in the API, so ignore this.