Re: [pve-devel] [PATCH v2 manager] fix #4481: fetch changelogs for any Proxmox repository

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

Am 30/01/2023 um 11:59 schrieb Leo Nunner:
> This patch fixes the issue that when the user supplied any non-standard
> repositories, the changelogs often wouldn't load. For example, providing
> both pve-no-subscription and pbs-no-subscription broke the changelog
> API, since the URL built for pbs-no-subscription was invalid.
> 

Can you specifiy a bit more specific example please?

we really should add the matching hierarchy on our repo CDN, then this wouldn't
be required at all and `apt changelog` would work too..

> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> Changes from v1:
>     - Directly pass $pkgfile to get_changelog_url
>     - Only check enabled repositories
>     - Compare the site of the package with the repository URL
> 
>  PVE/API2/APT.pm | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
> index 09c76545..c4e2bb3d 100644
> --- a/PVE/API2/APT.pm
> +++ b/PVE/API2/APT.pm
> @@ -89,11 +89,16 @@ my $get_pkgfile = sub {
>  };
>  
>  my $get_changelog_url =sub {
> -    my ($pkgname, $info, $pkgver, $origin, $component) = @_;
> +    my ($pkgname, $info, $pkgver, $pkgfile) = @_;
>  
>      my $changelog_url;
>      my $base;
>      $base = dirname($info->{FileName}) if defined($info->{FileName});
> +
> +    my $origin = $pkgfile->{Origin};
> +    my $component = $pkgfile->{Component};
> +    my $site = $pkgfile->{Site};

nit, could use a perl hash slice:

my ($origin, $component, $site) = $pkgfile->@{'Origin', 'Component', 'Site'};

albeit you use $component and $site only once and that without any alteration, so they're
effectively an useless intermediate variable and could be inlined too.


> +
>      if ($origin && $base) {
>  	$pkgver =~ s/^\d+://; # strip epoch
>  	my $srcpkg = $info->{SourcePkg} || $pkgname;
> @@ -101,10 +106,19 @@ my $get_changelog_url =sub {
>  	    $base =~ s!pool/updates/!pool/!; # for security channel
>  	    $changelog_url = "http://packages.debian.org/changelogs/$base/${srcpkg}_${pkgver}/changelog";
>  	} elsif ($origin eq 'Proxmox') {
> -	    if ($component eq 'pve-enterprise') {
> -		$changelog_url = "https://enterprise.proxmox.com/debian/$base/${pkgname}_${pkgver}.changelog";
> -	    } else {
> -		$changelog_url = "http://download.proxmox.com/debian/$base/${pkgname}_${pkgver}.changelog";
> +	    my $data = Proxmox::RS::APT::Repositories::repositories("pve");

To be sure, I still cannot check the changelog of a package like proxmox-backup-server
via the PVE web UI though?

> +
> +	    for my $file ($data->{files}->@*) {
> +		for my $repo ($file->{repositories}->@*) {
> +		    if (
> +			$repo->{Enabled}
> +		        && grep(/$component/, $repo->{Components}->@*)
> +		        && $repo->{URIs}[0] =~ m/$site/

mixed indentation, maybe we'd be better off using `next if expr;` stanzas instead
for readabillity

> +		    ) {
> +		        $changelog_url = $repo->{URIs}[0] . "/$base/${pkgname}_${pkgver}.changelog";
> +		        last;
> +		    }
> +		}
>  	    }
>  	}
>      }
> @@ -124,7 +138,7 @@ my $assemble_pkginfo = sub {
>      if (my $pkgfile = &$get_pkgfile($candidate_ver)) {
>  	$data->{Origin} = $pkgfile->{Origin};
>  	my $changelog_url = $get_changelog_url->(
> -	        $pkgname, $info, $candidate_ver->{VerStr}, $pkgfile->{Origin}, $pkgfile->{Component});
> +	        $pkgname, $info, $candidate_ver->{VerStr}, $pkgfile);
>  
>  	$data->{ChangeLogUrl} = $changelog_url if $changelog_url;
>      }
> @@ -431,7 +445,7 @@ __PACKAGE__->register_method({
>  	my $url;
>  
>  	die "changelog for '${pkgname}_$ver->{VerStr}' not available\n"
> -	    if !($pkgfile && ($url = &$get_changelog_url($pkgname, $info, $ver->{VerStr}, $pkgfile->{Origin}, $pkgfile->{Component})));
> +	    if !($pkgfile && ($url = &$get_changelog_url($pkgname, $info, $ver->{VerStr}, $pkgfile)));
>  
>  	my $data = "";
>