Add a new feature plugin to verify package installation#4660
Add a new feature plugin to verify package installation#4660vaibhavdaren wants to merge 10 commits intomainfrom
Conversation
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
ffe8028 to
5ae12a0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a verify-installation prepare plugin to verify package installation sources. The implementation includes abstractions in package managers, the plugin itself, its schema, and tests. The changes are well-structured. One minor improvement is suggested for a warning message.
Note: Security Review did not run due to the size of the PR.
vaibhavdaren
added
the
plugin | artifact
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
45bde38 to
2a3ab0f
Compare
thrix
left a comment
thrix
left a comment
There was a problem hiding this comment.
Nice plugin addition for verifying package sources. The overall structure follows tmt patterns well (container data class, normalization, schema, beakerlib tests). A few issues to address:
Must fix:
- Verification failures don't actually stop test execution - The plugin records FAIL results but doesn't raise
PrepareErroror append tooutcome.exceptions. The prepare step framework only aborts on exceptions, not on FAIL results. This contradicts the docstring promise. expected_reposhould beexpected-repo(kebab-case) - tmt convention uses hyphens in user-facing YAML keys. The underscore form is inconsistent withdist-git-source,is-virtualized, etc.
Should verify:
3. dnf5 / yum compatibility for repoquery --queryformat "%{from_repo}" - Dnf5Engine and YumEngine inherit this without override.
Checklist items still unchecked:
- Update the specification
- Adjust plugin docstring
- Mention the version
- Include a release note (file exists but version not mentioned)
|
Addressed comments from code review in f128998. |
vaibhavdaren
added
the
ci | full test
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
6a3a552 to
f128998
Compare
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
2 times, most recently
from
8ad17a3 to
b660235
Compare
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
b1927db to
1462e47
Compare
| rlPhaseStartTest "Test successful verification on dnf4" | ||
| rlLog "Verify packages installed from BaseOS and Docker CE repo pass verification" | ||
|
|
||
| rlRun -s "tmt run -i \$run/success --scratch -vvv --all \ | ||
| plan --name /plan/success \ | ||
| provision -h \$PROVISION_HOW --image \$TEST_IMAGE_PREFIX/centos/stream10/upstream:latest" \ | ||
| 0 "Run verification test with correct repos" | ||
|
|
||
| rlAssertGrep "pass .* / make" $rlRun_LOG | ||
| rlAssertGrep "pass .* / diffutils" $rlRun_LOG | ||
| rlAssertGrep "pass .* / docker-ce-cli" $rlRun_LOG | ||
| rlAssertGrep "All packages verified successfully." $rlRun_LOG | ||
| rlAssertGrep "1 test passed" $rlRun_LOG | ||
| rlPhaseEnd |
There was a problem hiding this comment.
Please not like this with so much duplication. If needed, please use anything in epel10 and include epel feature. Can try centpkg for example. It doesn't use or test brew but that would be fine for now.
There was a problem hiding this comment.
It is still unnecessarily split between dnf4 and dnf5. Unify them and try to make it similar to /tests/prepare/install. Some adjust using the distro and such would be better
- Change verify field from comma-separated strings to YAML dict format with package and expected_repo keys; remove CLI counterpart for now - Batch-query all packages at once using get_installed_repos() - Record per-package failures as PhaseResult in outcome instead of raising PrepareError; let the framework handle failure reporting - Fail hard on unsupported package managers instead of warning and skipping - Fix docstrings to match surrounding style - Update schema, test data and test assertions accordingly
- Track verification failures with a local flag instead of rescanning outcome.results to avoid false positives from super().go() results - Catch RunError from get_installed_repos and record as ERROR PhaseResult instead of propagating as an unhandled exception - Log unexpected repoquery output lines at debug level instead of silently ignoring them - Use str(m.package) in serialize to avoid Package subclass leaking into serialized output - Remove redundant test assertion for 'make-devel' already covered by 'fail make-devel' check
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
01bd2e9 to
a1ea9d1
Compare
tmt/package_managers/__init__.py
Outdated
| parts = line.split(maxsplit=1) | ||
| if len(parts) == 2: | ||
| result[parts[0]] = parts[1].strip() | ||
| elif len(parts) == 1 and parts[0] in result: |
There was a problem hiding this comment.
parts[0] in result is unnecessary in various ways. Also add comments on what each of these cases represent
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
dddef12 to
87ebdc0
Compare
vaibhavdaren
force-pushed
the
vaibhav-package-verify-plugin
branch
from
87ebdc0 to
f1c44c7
Compare
|
@vaibhavdaren btw tests are still failing hard? or will that be a focus at the end? Anyway, LGTM from my end now see below :) |
thrix
left a comment
thrix
left a comment
There was a problem hiding this comment.
Re-review after 10 iterations. All issues from my previous CHANGES_REQUESTED review have been resolved:
- Failures not stopping execution — Fixed:
outcome.exceptionspopulated alongside FAIL results (with FIXME for #4667) - kebab-case naming — No longer applies: schema simplified to plain
dict[str, str] - dnf5/yum compatibility — Addressed:
YumEngineraisesNotImplementedErrorwith comment; dnf5 UUIDs passed through as-is - NotImplementedError handler — Fixed: now emits
PhaseResult(ERROR)+ appends tooutcome.exceptions, consistent withGeneralErrorhandling
The plugin is well-structured — clean architecture (engine → manager → plugin), proper per-package PASS/FAIL results, consistent error handling. Tests cover success and failure paths on both CentOS (dnf4) and Fedora (dnf5).
One remaining item from @LecrisUT: unify data-centos/data-fedora into a single test using adjust with distro conditions instead of separate directories and if rlIsFedora branching.
Nice work iterating through the extensive review feedback @vaibhavdaren!
Generated-by: Claude Code
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| result: dict[str, Optional[str]] = dict.fromkeys(packages) | ||
| script = self.engine.get_package_origin(result.keys()) | ||
| output = self.guest.execute(script) | ||
| for line in (output.stdout or '').strip().splitlines(): |
There was a problem hiding this comment.
There are some loose ends I can't sort out:
- the dnf5/dnf4 notes below in the base class implementation suggest that dnf4 probably needs its own custom implementation, an implementation aware of dnf4 quirks.
- but then again, what's the contract between
engine.get_package_origin()and its callers, what's the expected output? A line per package, in thepackage originformat - if the dnf4 implementation of this method sneaks in empty lines, it's violating the expectations. This may be problem for commands building images: engine is asked to create a command with expected output, the command would be then executed in container image build time, and the dnf4 command would produce unexpected output, possibly breaking further processing. Maybe we could extend the contract, to make it more liberal: a line per package, in thepackage originformat, empty lines are ignored. And we no longer need to worry about dnf4 in the base class, dnf4 engine would produce valid script. - the "not installed via a repo" might be more common, other package managers may support installation of packages from files, therefore this might be worth an extension to the
get_package_origin()contract: a line per package, in thepackage originformat, empty lines are ignored,originis optional and if not specified, it means "unknown".
After this, we would get a simpler base implementation, fewer notes about dnf4 quirks in the base class, nicer protocol for get_package_origin(), and the need for a custom dnf4 implementation would be gone.
| result[parts[0]] = parts[1].strip() | ||
| elif len(parts) == 1: | ||
| # dnf4: empty %{from_repo} means package was not installed via a repo | ||
| result[parts[0]] = "[unknown]" |
There was a problem hiding this comment.
Deserves to be a module-level constant.
6 participants
Pull Request Checklist: #4644