From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C8FF069F02 for ; Wed, 24 Mar 2021 11:06:21 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BB3D737063 for ; Wed, 24 Mar 2021 11:06:21 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 60E6437057 for ; Wed, 24 Mar 2021 11:06:20 +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 2C08A42DFC for ; Wed, 24 Mar 2021 11:06:20 +0100 (CET) Date: Wed, 24 Mar 2021 11:06:13 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , pbs-devel@lists.proxmox.com References: <20210322115945.1362-1-f.ebner@proxmox.com> <1616493992.n5qqqz9oq6.astroid@nora.none> <93bd0e38-56fb-70eb-1379-9b736b5fb7b5@proxmox.com> In-Reply-To: <93bd0e38-56fb-70eb-1379-9b736b5fb7b5@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1616579721.0zge6ski36.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.123 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Mar 2021 10:06:21 -0000 snipped ;) On March 24, 2021 10:40 am, Fabian Ebner wrote: > Am 23.03.21 um 11:29 schrieb Fabian Gr=C3=BCnbichler: >> the "options" part is missing support for the following "feature" ("man >> sources.list"): >>=20 >> Multivalue options also have -=3D and +=3D as separators, which instea= d of >> replacing the default with the given value(s) modify the default >> value(s) to remove or include the given values. >>=20 >> I haven't tested that one though ;) >>=20 >=20 > The parser just uses "option+" and "option-" as the option keys then. It=20 > /could/ be interpreted on parsing, but if the user chose to use this=20 > notation, there's probably a reason and it should be written out the=20 > same way it came in again. I'll add a test though. yeah, makes sense (in a way). >=20 >> if a file cannot be parsed or is malformed (e.g. because I put "Uris" >> instead of "URIs" in a .sources file ;)), the whole API call fails with >> 400. it might be more user-friendly to mark indiviual .list/.sources >> files as containing invalid entries which are not displayed, and still >> return the rest? might make the result less actionable since we don't >> have the complete picture, but it still might be better than a single >> error message for one of X files.. >>=20 >=20 > I did consider something like this for a bit, but not sure how to=20 > cleanly organize the API then. The call should still error out in my=20 > opinion and syntactic errors should be rather rare anyways (apt also=20 > just complains when it cannot parse). We could continue parsing and=20 > collect all the errors at once at least, but not sure if that's worth it? I was thinking more of further inconsistencies between APT and our=20 parser like the case-sensitivity issue. like you said, APT will just=20 ignore a wrong/broken entry or file, and not error out altogether. so=20 maybe it would make sense to mimic that behaviour -> add a=20 warnings/error field, and let the caller decide whether it just wants to=20 display that or the (partial) result + the additional information. e.g.,=20 we could disable editing via the GUI for invalid files, but still allow=20 it for other files. IMHO for this the request as a whole does not have to error out on the=20 API/HTTP level if the parser encounters something it does not=20 understand. >> we have a warning for Debian unstable, but none for Debian testing which >> should also never be enabled on a production machine. >>=20 >=20 > If there is an "upgrade suites" button/API call, then there would be=20 > warnings after using that. But since enabling that button/API call needs=20 > to happen anyways before each major release, I guess removing the e.g.=20 > 'bullseye' warnings then is just one more place to touch. no, because 'bullseye' is not 'testing' ;) or at least, they should not=20 be treated the same. having 'bullseye' as a suite is fine when upgrading=20 from 'buster' to 'bullseye'. having 'testing' there is never good on a=20 stable/production system. > Or maybe add a 'before_major_release' parameter to check_repositories=20 > and also to the UI? Then only the product specific code needs to be=20 > touched before each major release. Of course there still needs to be a=20 > new version of the library for after each major release. the check for 'bullseye' could contain text that indicates that the=20 warning is benign if you are preparing the major release upgrade? or the=20 'upgrade suites' button sets a flag somewhere (comment? ;)), and that=20 then automatically skips that check for the given suite? >> we might want to match known-official repo URIs (ftp.*.debian.org, >> deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark >> "potentially dangerous external repositories" (or to give the official >> ones a "official mirror" badge or something like that) >>=20 >=20 > Might be done as part of the check_repositories call if we go for=20 > warnings. If we go for badges, we'd need something new or change the=20 > interface for check_repositories. I feel like badges would be preferable=20 > though... badges are nicer since they just highlight "this is good", which is=20 easier to determine than "this is bad" (which is actually just "this=20 might be bad"). having some custom repo that is okay is not that=20 uncommon (internal software, monitoring, ansible integration packages,=20 ..), so being too noisy might be a bad idea. =