Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

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

Am 06.11.23 um 11:38 schrieb Dominik Csapak:
> On 11/6/23 11:12, Fiona Ebner wrote:
>> Am 06.11.23 um 10:34 schrieb Dominik Csapak:
>>> On 11/6/23 10:22, Fiona Ebner wrote:
>>>> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>>>>> +my $defaultData = {
>>>>> +    propertyList => {
>>>>> +    type => { description => 'Profile type.' },
>>>>> +    id => {
>>>>> +        type => 'string',
>>>>> +        description => "The ID of the profile.",
>>>>> +        format => 'pve-configid',
>>>>> +    },
>>>>
>>>> The ID is usually not a property AFAIK. Doesn't this lead to
>>>> duplication
>>>> when writing the section config, i.e.
>>>>
>>>> type: <ID>
>>>>      id <ID>
>>>>
>>>> ? Do we gain anything by having it be a property?
>>>
>>> mhm? the id has to be part of the properties, otherwise
>>> the generated api with 'createSchema' etc. would not include it.
>>>
>>> (it isn't always named id, e.g. in the storage plugins
>>> it's 'storage')
>>>
>>
>> I was just reminded of [0], where it could lead to that situation. Would
>> need to check if that patch still applies, because since then
>> Jobs/RealmSync.pm has been added.
>>
>> But somebody needs to filter the 'storage' property, right? Isn't that
>> property actually superfluous?
> 
> well, yes and no,
> 
> in a section config, we always have a global 'propertyList'
> and a per type 'options' list (which only references the propertylist)
> 
> when using the 'create/updateSchema' calls, *all* options from the
> propertylist
> will be included (incl. the type) so the id is always there

Well, yes. If it was declared explicitly in the propertyList ;)

I checked some other section configs and they all have an id (not always
named id) property, so the question is what to do about the FIXME comment:

https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Job/Registry.pm;h=32e02728d629dca67bc479a0c25d3ea3aae2858e;hb=HEAD#l18

Should we just remove the comment, because it's more consistent to other
section configs with the ID rather than without?

For the other one

https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Job/Registry.pm;h=32e02728d629dca67bc479a0c25d3ea3aae2858e;hb=HEAD#l69

we can just drop the auto-injection, because neither VZDump/JobBase.pm
nor Jobs/RealmSync.pm declare 'id' in their 'options', so the if
condition is never true AFAICT.

> 
> in the api calls, we then use this id, to modify the config
> but it isn't actually contained in the 'options' of any type
> and since we extract it from the parameters, it does not actually
> land in the config part
> 
> (ofc this is a bit error-prone, and forgetting to extract it would
> lead to the situation you describe)
> 

Okay, I see.