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

[Fabian Ebner <f.ebner@proxmox.com>]

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<APTRepository>),
	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.