Enhance warning about RPMs that were not validate by RPM #1459
Conversation
There was a problem hiding this comment.
Thanks for the patch. I have just a few minor notes that might be a matter of preference.
Also, the commit message says that "the ID of repository provides a good hint", but it doesn't say why. Can you please mention it's because the check is skipped per repo?
libdnf5/base/transaction.cpp
Outdated
| _("Warning: skipped PGP checks for {0} package(s) from {1} repository(s)."), | ||
| num_checks_skipped, | ||
| repo_string); |
There was a problem hiding this comment.
The "(s)" for "repository" doesn't work as well as for "package". Firstly, it doesn't match the plural form, secondly, it's less clear when there is a list of repositories and not just a number. For example, this reads a bit weird: "skipped PGP checks for 82 package(s) from fedora, updates repository(s)." I would change it, so that the list of repos is at the end: "skipped PGP checks for 82 package(s) from repositories: fedora, updates". What do you think?
There was a problem hiding this comment.
We can even leverage plural forms here:
P_("skipped PGP checks for {} package from repository {}", "skipped PGP check for {} packages from repositories: {}", num_checks_skipped)
Or something similar, plurals translations are currently not used in dnf5 so this would be a pioneer :)
There was a problem hiding this comment.
I was thinking about P_, but there are 3 possibility
"skipped PGP checks for {} package from repository {}"
"skipped PGP checks for {} packages from repository {}"
"skipped PGP checks for {} packages from repositories {}"
There was a problem hiding this comment.
Yeah, this is not exactly a straightforward example for P_ as it contains two plurals. The only way would be splitting the message into two parts - one for package(s) and the other for repository(ies).
I'm not sure if this complication for translators is worth it, as in some languages it might be difficult to join such split sentences back together.
There was a problem hiding this comment.
utils::sformat(
P_(
"skipped PGP checks for {} package from repositories: {}",
"skipped PGP check for {} packages from repositories: {}",
num_checks_skipped),
num_checks_skipped
repo_string);
libdnf5/base/transaction.cpp
Outdated
| @@ -1048,6 +1048,7 @@ bool Transaction::Impl::check_gpg_signatures() { | |||
| libdnf5::rpm::RpmSignature rpm_signature(base); | |||
| std::set<std::string> processed_repos{}; | |||
| int num_checks_skipped = 0; | |||
| std::set<std::string> repo_of_checks_skipped; | |||
There was a problem hiding this comment.
I don't get this variable name. Wouldn't something like repos_with_skipped_checks be more intuitive?
libdnf5/base/transaction.cpp
Outdated
| @@ -1098,7 +1100,11 @@ bool Transaction::Impl::check_gpg_signatures() { | |||
| } | |||
| } | |||
| if (num_checks_skipped > 0) { | |||
| auto warning_msg = utils::sformat(_("Warning: skipped PGP checks for {} package(s)."), num_checks_skipped); | |||
| auto repo_string = libdnf5::utils::string::join(repo_of_checks_skipped, ", "); | |||
There was a problem hiding this comment.
Please mark ", " for translation. The joiner is language-specific.
|
The PR and tests were updated according to suggestions. |
DNF5 informs about number of packages that signature was not verified, but without any additional detail. The ID of repository provides a good hint for user why the check was skipped. The behavior is related to configuration options which some of them are repo specific or specific for commandline repository. If user wants to verify everything, the hint provides sufficient information which configuration of repository should be modified. Closes: rpm-software-management#1311
a889d09
DNF5 informs about number of packages that signature was not verified, but without any additional detail. The ID of repository provides a good hint for user why the check was skipped.
Closes: #1311
Requires CI modification: rpm-software-management/ci-dnf-stack#1498