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

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

Am 23.03.21 um 11:29 schrieb Fabian Grünbichler:
> On March 22, 2021 12:59 pm, Fabian Ebner wrote:
>> List the configured repositories and have some basic checks for them.
>>
>> The plan is to use perlmod to make the Rust implementation available for PVE+PMG
>> as well.
>>
>> There's still the question if introducing a digest is worth it. At the moment,
>> the warnings returned by the checkrepositories call might not match up with the
>> repositories returned previously, but that's a rather minor issue as editing
>> repositories is a rare operation.
>> Should a digest be added now to be future-proof? Should it live in the
>> proxmox-apt crate and be file-level, or would it be enough to hash the result in
>> the PBS API call and use that? The latter seems like the more pragmatic approach
>> and avoids cluttering the APT backend.
> 
> gave this a spin, looks mostly good to me, some things I noticed:
> 
> "man deb822" states:
> 
>   Field names are not case-sensitive, but it is usual to capitalize the
>   field names using mixed case as shown below. Field values are
>   case-sensitive unless the description of the field says otherwise.
> 
> the parser here is case-sensitive and chokes on .sources files which APT
> happily parses & uses.
> 

Thanks for catching that, I'll fix this in the next version.

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

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

> the GUI already makes the API request when opening the 'Administration'
> part, which then displays an error modal if any of the files is wrong -
> even though we are not on the repositories tab at that moment. this is
> confusing. also, opening the repositories tab triggers a (re?)load
> anyhow..
> 

Surely not optimal. Hopefully easily fixed by removing the 'reload' call 
in 'initComponent'. Too used to having active on init...

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

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.

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

> a digest based on raw file contents or some other stable means seems
> sensible to me. hashing just the results of the parser is usually wrong
> (digest mismatch with different parser versions, parser bugs causing
> indeterminism with rare setups, ...).
> 

I'll try and add a digest that's returned when parsing from the raw 
contents and then the API calls can re-compare against that before doing 
any further calls to the library.

>>
>>
>> Changes from v2:
>>      * incorporate Wolfgang's feedback
>>      * improve main warning's UI
>>
>> Still missing:
>>      * Upgrade suite/distribuiton button to be used before major release
>>        upgrades (but it's really simply to add that now).
>>      * perlmod magic and integration in PVE and PMG.
>>
>> Changes v1 -> v2:
>>      * Perl -> Rust
>>      * PVE -> PBS
>>      * Don't rely on regexes for parsing.
>>      * Add writer and tests.
>>      * UI: pin warnings to the repository they're for.
>>      * Keep order of options consistent with configuration.
>>      * Smaller things noted on the individual patches.
>>
>>
>> proxmox-apt:
>>
>> Fabian Ebner (4):
>>    initial commit
>>    add files for Debian packaging
>>    add functions to check for Proxmox repositories
>>    add check_repositories function
>>
>>
>> proxmox-backup:
>>
>> Fabian Ebner (4):
>>    depend on new proxmox-apt crate
>>    api: apt: add repositories call
>>    ui: add repositories
>>    add check_repositories_call
>>
>>   Cargo.toml                  |  1 +
>>   src/api2/node/apt.rs        | 78 +++++++++++++++++++++++++++++++++++++
>>   www/ServerAdministration.js |  7 ++++
>>   3 files changed, 86 insertions(+)
>>
>>
>> widget-toolkit:
>>
>> Fabian Ebner (2):
>>    add UI for APT repositories
>>    APT repositories: add warnings
>>
>>   src/Makefile                |   1 +
>>   src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 296 insertions(+)
>>   create mode 100644 src/node/APTRepositories.js
>>
>> -- 
>> 2.20.1
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>