From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <t.lamprecht@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id F298A911D2
 for <pve-devel@lists.proxmox.com>; Wed,  1 Feb 2023 10:15:48 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id D41C72CFFA
 for <pve-devel@lists.proxmox.com>; Wed,  1 Feb 2023 10:15:18 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pve-devel@lists.proxmox.com>; Wed,  1 Feb 2023 10:15:17 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B6F1E45989
 for <pve-devel@lists.proxmox.com>; Wed,  1 Feb 2023 10:15:16 +0100 (CET)
Message-ID: <44d61a5e-87cb-9c3d-788e-6bb5f865ef18@proxmox.com>
Date: Wed, 1 Feb 2023 10:15:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:110.0) Gecko/20100101
 Thunderbird/110.0
Content-Language: en-GB, de-AT
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
 Leo Nunner <l.nunner@proxmox.com>
References: <20230130105953.126483-1-l.nunner@proxmox.com>
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
In-Reply-To: <20230130105953.126483-1-l.nunner@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.005 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A            -0.09 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH v2 manager] fix #4481: fetch changelogs for
 any Proxmox repository
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 01 Feb 2023 09:15:49 -0000

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 = "";
>