Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 24, 2021 1:08 pm, Fabian Ebner wrote:
> 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:
>>>> 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<APTRepository>),
> 	Error(String),
> }
> 
> if we go for a file-level approach.

yeah, I meant 'partial' as in return the parser results for good files, 
and 'don't know how to parse this' for the bad files. sorry for not 
being more explicit.

> 
>>>> 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.

parameter is okay as well - I was just pointing out other potential 
solutions. ideally we'd have some sort of "admin says they want to 
upgrade" and after that we treat bullseye repos as okay, as opposed to 
"PVE 6.x is recent enough to upgrade" already triggering that. how we 
get there is largely irrelevant ;)