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 6C6996A068 for ; Wed, 24 Mar 2021 13:40:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 51CB99A1C for ; Wed, 24 Mar 2021 13:40:00 +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 EB5839A10 for ; Wed, 24 Mar 2021 13:39:58 +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 AF59042577 for ; Wed, 24 Mar 2021 13:39:58 +0100 (CET) Date: Wed, 24 Mar 2021 13:39:51 +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> <1616579721.0zge6ski36.astroid@nora.none> <7cb601f7-ad5c-ab41-aa61-928d3107ddec@proxmox.com> In-Reply-To: <7cb601f7-ad5c-ab41-aa61-928d3107ddec@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1616589353.tdtcrjirrr.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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:40:30 -0000 On March 24, 2021 1:08 pm, Fabian Ebner wrote: > Am 24.03.21 um 11:06 schrieb Fabian Gr=C3=BCnbichler: >> snipped ;) >>=20 >> On March 24, 2021 10:40 am, Fabian Ebner wrote: >>> Am 23.03.21 um 11:29 schrieb Fabian Gr=C3=BCnbichler: >>>> 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 wit= h >>>> 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 i= t? >>=20 >> 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. >>=20 >> 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. >>=20 >=20 > apt seems to error out on the first problem for each file, and it will=20 > error out before doing anything else when the parsing failed. >=20 > Is it really worth keeping the partial result in the error case? I feel=20 > like it'd be cleaner to have something like >=20 > enum APTFileResult { > Ok(Vec), > Error(String), > } >=20 > if we go for a file-level approach. yeah, I meant 'partial' as in return the parser results for good files,=20 and 'don't know how to parse this' for the bad files. sorry for not=20 being more explicit. >=20 >>>> we have a warning for Debian unstable, but none for Debian testing whi= ch >>>> 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 need= s >>> to happen anyways before each major release, I guess removing the e.g. >>> 'bullseye' warnings then is just one more place to touch. >>=20 >> 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. >>=20 >>> 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. >>=20 >> 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? >>=20 >=20 > I'd be in favor of the benign warning message, but the problem is that=20 > it would be displayed once for every repository. The comment flag seems=20 > a bit hacky, who would remove that again after the upgrade? As one=20 > should always update to the last version of the current suite before=20 > attempting a major upgrade, and because we need to touch the code to=20 > enable the button/API call anyways, why is my suggestion with the=20 > parameter not good enough? And just a quick idea that would need to be=20 > fleshed out: having a major_upgrade_possible file packaged somewhere and=20 > a major_update_possible API call, would lead to a single place to touch. parameter is okay as well - I was just pointing out other potential=20 solutions. ideally we'd have some sort of "admin says they want to=20 upgrade" and after that we treat bullseye repos as okay, as opposed to=20 "PVE 6.x is recent enough to upgrade" already triggering that. how we=20 get there is largely irrelevant ;) =