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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EFB8C6A04B for ; Wed, 24 Mar 2021 13:08:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DAAD59448 for ; Wed, 24 Mar 2021 13:08:07 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 748E5940F for ; Wed, 24 Mar 2021 13:08:06 +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 4274E42EB4 for ; Wed, 24 Mar 2021 13:08:06 +0100 (CET) To: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= , 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> <1616579721.0zge6ski36.astroid@nora.none> From: Fabian Ebner Message-ID: <7cb601f7-ad5c-ab41-aa61-928d3107ddec@proxmox.com> Date: Wed, 24 Mar 2021 13:08:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <1616579721.0zge6ski36.astroid@nora.none> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.143 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 12:08:38 -0000 Am 24.03.21 um 11:06 schrieb Fabian Grünbichler: > snipped ;) > > On March 24, 2021 10:40 am, Fabian Ebner wrote: >> Am 23.03.21 um 11:29 schrieb Fabian Grünbichler: >>> the "options" part is missing support for the following "feature" ("man >>> sources.list"): >>> >>> Multivalue options also have -= and += as separators, which instead of >>> replacing the default with the given value(s) modify the default >>> value(s) to remove or include the given values. >>> >>> I haven't tested that one though ;) >>> >> >> The parser just uses "option+" and "option-" as the option keys then. It >> /could/ be interpreted on parsing, but if the user chose to use this >> notation, there's probably a reason and it should be written out the >> same way it came in again. I'll add a test though. > > yeah, makes sense (in a way). > >> >>> 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.. >>> >> >> I did consider something like this for a bit, but not sure how to >> cleanly organize the API then. The call should still error out in my >> opinion and syntactic errors should be rather rare anyways (apt also >> just complains when it cannot parse). We could continue parsing and >> 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 > parser like the case-sensitivity issue. like you said, APT will just > ignore a wrong/broken entry or file, and not error out altogether. so > maybe it would make sense to mimic that behaviour -> add a > warnings/error field, and let the caller decide whether it just wants to > display that or the (partial) result + the additional information. e.g., > we could disable editing via the GUI for invalid files, but still allow > it for other files. > > IMHO for this the request as a whole does not have to error out on the > API/HTTP level if the parser encounters something it does not > understand. > apt seems to error out on the first problem for each file, and it will error out before doing anything else when the parsing failed. Is it really worth keeping the partial result in the error case? I feel like it'd be cleaner to have something like enum APTFileResult { Ok(Vec), Error(String), } if we go for a file-level approach. >>> we have a warning for Debian unstable, but none for Debian testing which >>> should also never be enabled on a production machine. >>> >> >> If there is an "upgrade suites" button/API call, then there would be >> warnings after using that. But since enabling that button/API call needs >> to happen anyways before each major release, I guess removing the e.g. >> 'bullseye' warnings then is just one more place to touch. > > no, because 'bullseye' is not 'testing' ;) or at least, they should not > be treated the same. having 'bullseye' as a suite is fine when upgrading > from 'buster' to 'bullseye'. having 'testing' there is never good on a > stable/production system. > >> Or maybe add a 'before_major_release' parameter to check_repositories >> and also to the UI? Then only the product specific code needs to be >> touched before each major release. Of course there still needs to be a >> new version of the library for after each major release. > > the check for 'bullseye' could contain text that indicates that the > warning is benign if you are preparing the major release upgrade? or the > 'upgrade suites' button sets a flag somewhere (comment? ;)), and that > then automatically skips that check for the given suite? > I'd be in favor of the benign warning message, but the problem is that it would be displayed once for every repository. The comment flag seems a bit hacky, who would remove that again after the upgrade? As one should always update to the last version of the current suite before attempting a major upgrade, and because we need to touch the code to enable the button/API call anyways, why is my suggestion with the parameter not good enough? And just a quick idea that would need to be fleshed out: having a major_upgrade_possible file packaged somewhere and a major_update_possible API call, would lead to a single place to touch. >>> 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) >>> >> >> Might be done as part of the check_repositories call if we go for >> warnings. If we go for badges, we'd need something new or change the >> interface for check_repositories. I feel like badges would be preferable >> though... > > badges are nicer since they just highlight "this is good", which is > easier to determine than "this is bad" (which is actually just "this > might be bad"). having some custom repo that is okay is not that > uncommon (internal software, monitoring, ansible integration packages, > ..), so being too noisy might be a bad idea. > I'll try and hope to find a clean way to adapt the check_repositories interface.