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

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

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.

> 
> [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.

>  __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