Re-add --best to repoclosure - #436
Conversation
That explanation is bullshit. Repoclosure "just doesn't work" with modules properly, as it has no way to know which module needs to be enabled and which not. Setting I am all for enabling |
I'll write the one about parrot down. That's a great parallel! I verify most of the LLM output, but I rushed with this one without actually verifying what it says. So, next time. Maybe in #437? 😉 |
Without --best, dnf repoclosure can silently resolve a Requires against an older, still-present package instead of the newest one, masking real reverse-dependency breaks at PR time (e.g. theforeman/foreman-packaging#13647 bumping rubygem-foreman-tasks to 13.0.0 while rubygem-foreman_ansible still required < 13 — per-PR repoclosure passed, the break only surfaced via staging repoclosure the next day and was fixed in #13661). --best was dropped in theforeman#246 (2020) because it broke EL8 module repoclosure. module_hotfixes=1, since added to the modular repos in foreman-packaging's yum.conf, already neutralizes that problem. Verified: with a local dnf4 (CentOS Stream 9, matching this project's own container) reproduction using synthetic RPMs mirroring the foreman-tasks/foreman_ansible incident, --best correctly flags the break that is otherwise silently missed. Re-checked today's real el8-foreman-client and el9-foreman-plugins staging targets with their full lookaside sets and got clean, unaffected results. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
28cd17e to
a71a53d
Compare
zjhuntin
left a comment
zjhuntin
left a comment
There was a problem hiding this comment.
Review: Re-add --best to repoclosure
Scope: 2 files changed, +5/-0 lines | Adds --best flag to the dnf repoclosure command
Summary
This PR re-adds the --best flag to dnf repoclosure that was dropped in PR #246 (2020) due to EL8 module conflicts. The original reason no longer applies since module_hotfixes=1 was added to the modular repos in foreman-packaging's repoclosure config. Without --best, dnf can silently resolve a Requires against an older package instead of the newest, masking real dependency breakage at PR time. Ready to merge.
Verdict: APPROVE
Critical Issues (must fix)
None.
Important Issues (should fix)
None.
Minor Suggestions (nice to have)
None — the change is exactly one line in the module and the corresponding test updates. There's nothing to simplify or improve.
zjhuntin
left a comment
zjhuntin
left a comment
There was a problem hiding this comment.
Review: Re-add --best to repoclosure
Scope: 2 files changed, +5/-0 lines | Adds --best flag to the dnf repoclosure command
Summary
This PR re-adds --best to the dnf repoclosure command in obal/data/modules/repoclosure.py. The flag was dropped in PR #246 (2020) because it broke EL8 module repoclosure, but that's been neutralized by module_hotfixes=1 in foreman-packaging's repoclosure/yum.conf. Without --best, dnf can silently resolve a Requires against an older package instead of the newest, masking real dependency breakage — as demonstrated by the foreman-packaging#13647 incident. Ready to merge.
Verdict: APPROVE
Critical Issues (must fix)
None.
Important Issues (should fix)
None.
Minor Suggestions (nice to have)
None. The code change is a single line addition at the right position in the command array (after --newest, grouping resolution-strategy flags together). All 4 repoclosure functional tests (test_obal_repoclosure, test_obal_repoclosure_with_downloaded_rpms, test_obal_repoclosure_katello_with_downloaded_rpms, test_obal_repoclosure_with_check_repo) are updated. The assert_in_mockbin_log helper does per-entry substring matching so the flag ordering in the test lists is flexible.
Not exactly the same, but https://gruhn.me/blog/2026-08-03/ introduces the term "meat proxy". |
…ry names
--newest --best (obal#436) resolves Requires against the newest package
across all repos while still checking every repo's own newest package in
isolation. A subpackage that self-pins via
"Requires: %{name} = %{version}-%{release}" (e.g. a "-doc"/"-cockpit"
sibling) then false-positives the instant a newer build of the same SRPM
lands anywhere else in the checked set, since the persistent staging
repo's stale copy has nothing left to satisfy its exact pin against.
Exclude the package under test from the persistent staging repos it's
checked against, since the PR's own freshly-built repo is authoritative
for it. Scoped with dnf's per-repo --setopt=<repo>.excludepkgs=, and
resolved to the package's exact binary names via rpmspec (never a glob)
so bumping e.g. "foreman" or "rubygem-hammer_cli" can't also exclude an
unrelated package that happens to share a name prefix and silently mask
a real cross-package regression - the exact failure mode #436 exists to
catch (theforeman/foreman-packaging#13647/#13661/#13688).
Reproduced against the real theforeman/foreman-packaging#13921/#13922
CI failures and the live el9-foreman-plugins-nightly-staging repo.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 participants
Summary
Without
--best,dnf repoclosurecan silently resolve aRequiresagainst an older, still-present package instead of the newest one — masking real reverse-dependency breaks at PR time instead of failing loudly.Real-world case: theforeman/foreman-packaging#13647 bumped
rubygem-foreman-tasksto 13.0.0 whilerubygem-foreman_ansiblestill hadRequires: (rubygem(foreman-tasks) >= 10.0 with rubygem(foreman-tasks) < 13). Per-PR COPR repoclosure passed anyway. The break wasn't caught until staging repoclosure ran the next day, fixed in theforeman/foreman-packaging#13661 (and its DEB-side twin, theforeman/foreman-packaging#13688).--bestwas originally dropped in #246 (2020) because it broke EL8 module repoclosure (module_hotfixeswasn't respected).module_hotfixes=1has since been added to the modular repos inforeman-packaging'srepoclosure/yum.conf, which already neutralizes that problem — so the original reason for dropping--bestno longer applies.Verification
Reproduced the actual incident with synthetic RPMs mirroring the
foreman-tasks/foreman_ansibleversions involved, run through the real dnf4 client (CentOS Stream 9, matching this project's ownDockerfile— not dnf5, whoserepoclosure --bestis a no-op alias for--newestand would give a false read here):--best:dnf repoclosureexits0— silently passes, reproducing the real miss.--best: exits1and correctly reports the unresolvedrubygem(foreman-tasks) < 13dependency.Also re-ran today's real
el8-foreman-client-nightly-stagingandel9-foreman-plugins-nightly-stagingtargets (with their full lookaside sets) with--best— clean, no new false positives.tests/test_functional.pyupdated to expect--bestin the generateddnf repoclosurecommand. Full suite: 126 passed.Related
foreman-packaging'spackage_manifest.yaml— no obal-side gap there.