Move config-manager install to package manager abstraction#4848
Move config-manager install to package manager abstraction#4848
config-manager install to package manager abstraction#4848Conversation
![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 introduces a unified install_config_manager method across package managers and refactors the CRB and EPEL features to use this abstraction. Add an immediately parameter to the install_config_manager method to support batching package installations in image mode.
LecrisUT
left a comment
LecrisUT
left a comment
There was a problem hiding this comment.
Sure. Only suggestion is to switch the order of the PRs
thrix
force-pushed
the
worktree-feature-config-manager-abstraction
branch
from
45bd5a1 to
cbaa1a0
Compare
thrix
force-pushed
the
worktree-feature-config-manager-abstraction
branch
from
cbaa1a0 to
0072821
Compare
|
/packit build |
|
/packit retest-failed |
thrix
added
ci | full test
| CONTAINER_CONFIG_MANAGER_MATRIX = [ | ||
| (container, pm_class) | ||
| for container, pm_class in CONTAINER_BASE_MATRIX | ||
| if pm_class | ||
| in ( | ||
| tmt.package_managers.dnf.Dnf, | ||
| tmt.package_managers.dnf.Dnf5, | ||
| tmt.package_managers.dnf.Yum, | ||
| ) | ||
| ] |
There was a problem hiding this comment.
There already is CONTAINER_BASE_MATRIX, please reuse it.
| CONTAINER_CONFIG_MANAGER_MATRIX_IDS = [ | ||
| f'{container.url} / {package_manager_class.__name__.lower()}' | ||
| for container, package_manager_class in CONTAINER_CONFIG_MANAGER_MATRIX | ||
| ] |
There was a problem hiding this comment.
Why not loop over CONTAINER_MATRIX_IDS?
| """ | ||
| return self.install(*installables, options=options) | ||
|
|
||
| def install_config_manager(self) -> None: |
There was a problem hiding this comment.
Just a naming issue: so far we're using assert_something() naming scheme for making sure something is available - assert_reachable(), _assert_rsync(), etc. It's not the best naming scheme, but I'd recommend to sticking to it so we can find the "make sure config manager tool is present" case later when we find a better name for such methods. Maybe assert_config_manager()?
There was a problem hiding this comment.
@happz renamed according to the suggestion
thrix
force-pushed
the
worktree-feature-config-manager-abstraction
branch
from
0072821 to
9b76c8a
Compare
thrix
force-pushed
the
worktree-feature-config-manager-abstraction
branch
from
9b76c8a to
fbde1a0
Compare
Follow the `copr_plugin` / `enable_copr()` pattern: add a `config_manager_plugin` ClassVar and `install_config_manager()` method to the package manager layer instead of duplicating `dnf-command(config-manager)` installation in feature plugins. - `Dnf`: `dnf-command(config-manager)` - `Dnf5`: `dnf5-command(config-manager)` - `Yum`: `yum-utils` Closes: #4847 Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
* refactor tests * rename function name Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
thrix
force-pushed
the
worktree-feature-config-manager-abstraction
branch
from
fbde1a0 to
4b3393c
Compare
5 participants
Follow the
copr_plugin/enable_copr()pattern: add aconfig_manager_pluginClassVar andinstall_config_manager()method to the package manager layer instead of duplicatingdnf-command(config-manager)installation in feature plugins.Dnf:dnf-command(config-manager)Dnf5:dnf5-command(config-manager)Yum:yum-utilsCloses: #4847
Assisted-by: Claude Code
Pull Request Checklist