Re: [pve-devel] [PATCH pve-manager v2 2/4] api: add APT versions return schema

[Nicolas Frey <n.frey@proxmox.com>]

On 10/2/25 10:50 AM, Thomas Lamprecht wrote:
> Am 24.09.25 um 13:59 schrieb n.frey@proxmox.com:
>> From: Nicolas Frey <n.frey@proxmox.com>
>>
>> listed only supported debian architectures [0] in enum for 'Arch'.
>> listed all APT package states in enum for 'CurrentState'.
> 
> This is a bit specific and the sentence seems like it starts
> somewhere in the middle, maybe this can be lead up to with
> adding a little bit more context that avoids that one starts to
> wonder what a APT version return schema has to do with Debian A+rchs.
> 

Will add context in v3, thanks!

>>
>> [0] https://wiki.debian.org/SupportedArchitectures
>>
>> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
>> ---
>>  PVE/API2/APT.pm | 113 ++++++++++++++++++++++++++++--------------------
>>  1 file changed, 67 insertions(+), 46 deletions(-)
>>
>> diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
>> index 0d07cf38..885b85ae 100644
>> --- a/PVE/API2/APT.pm
>> +++ b/PVE/API2/APT.pm
> 
>> @@ -769,6 +771,25 @@ __PACKAGE__->register_method({
>>      },
>>  });
>>  
>> +$apt_package_return_props->{CurrentState} = {
>> +    type => 'string',
>> +    description => 'Current state of the package installed on the system.',
>> +    # Possible CurrentState variants according to AptPkg::Cache
>> +    enum => [qw(Installed NotInstalled UnPacked HalfConfigured HalfInstalled ConfigFiles)],
>> +};
>> +
>> +$apt_package_return_props->{RunningKernel} = {
>> +    type => 'string',
>> +    description => "Kernel release, only for package 'proxmox-ve'.",
>> +    optional => 1,
>> +};
>> +
>> +$apt_package_return_props->{ManagerVersion} = {
>> +    type => 'string',
>> +    description => "Version of the currently running pve-manager API server.",
>> +    optional => 1,
>> +};
>> +
> 
> Modifying a hash reference that is already used above is rather dangerous, as just
> some code line movement can introduce different behavior and wrong return types.
> 
> You rather should create a new (cloned) variable, e.g.:
> 
> my $versions_api_return_schema = {
>     $apt_package_return_props->%*,
>     CurrentState => {
>         ...,
>     },
>     ...,
> }
> 
> As this is then only used once you can also do that inline below, as
> single-use intermediate variables are most often rather useless and just an
> extra indirection. While they sometimes the can indeed help bring clarity,
> here it would be pretty clear what's happening without that.

Thanks for the feedback! I had a feeling modifying the hash reference might
not be the best approach (still getting used to Perl).
Will address in v3.

> 
>>  __PACKAGE__->register_method({
>>      name => 'versions',
>>      path => 'versions',
>> @@ -788,7 +809,7 @@ __PACKAGE__->register_method({
>>          type => "array",
>>          items => {
>>              type => "object",
>> -            properties => {},
>> +            properties => sort $apt_package_return_props,
>>          },
>>      },
>>      code => sub {
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel