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

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

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.

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 ;)

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

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

we have a warning for Debian unstable, but none for Debian testing which 
should also never be enabled on a production machine.

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)

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

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