Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support DNF5's config-manager #5657

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

marusak
Copy link
Contributor

@marusak marusak commented May 20, 2024

Fedora rawhide is installing DNF5 on new systems. Payload that sets up multilib support on the system runs on the installed system, thus using DNF5. DNF5 changed config-manager plugin syntax. This commit makes sure that it works with both DNF4 as well as with DNF5.

@marusak
Copy link
Contributor Author

marusak commented May 20, 2024

Multilib test is not working ATM - see rhinstaller/kickstart-tests#1192

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the missing test and verify that the collect_requirements is correctly implemented. It seems to me that DNF payload might be solving this directly.

Otherwise looks great!

Comment on lines +480 to 494
def test_multilib_policy_dnf4(self, execute):
"""Update the multilib policy on pre-dnf5 systems."""
execute.return_value = 0

with tempfile.TemporaryDirectory() as sysroot:
data = PackagesConfigurationData()
data.multilib_policy = MULTILIB_POLICY_ALL
dnf_manager = Mock(spec=DNFManager)
dnf_manager.is_package_available.return_value = False

task = UpdateDNFConfigurationTask(sysroot, data)
task = UpdateDNFConfigurationTask(sysroot, data, dnf_manager)
task.run()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to check also if the required plugin package is correctly requested.

Comment on lines 375 to 393
def collect_requirements(self):
"""Return installation requirements for this module.

:return: a list of requirements
"""
requirements = []

if self.dnf_manager.is_package_available("dnf5"):
plugins_name = "dnf5-modules"
else:
plugins_name = "dnf-plugins-core"

if self._packages_configuration.multilib_policy != MULTILIB_POLICY_BEST:
requirements.append(
Requirement.for_package(plugins_name, reason="Needed to enable multilib support.")
)

return requirements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be the correct way at the end. I looked on the code and it seems that we should follow path:

return collect_remote_requirements() \

Copy link

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@pep8speaks
Copy link

pep8speaks commented Aug 14, 2024

Hello @marusak! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-08-15 13:55:24 UTC

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

pyanaconda/modules/payloads/payload/dnf/installation.py Outdated Show resolved Hide resolved
Fedora rawhide is installing DNF5 on new systems. Payload that sets up
multilib support on the system runs on the installed system, thus using
DNF5. DNF5 changed config-manager plugin syntax. This commit makes sure
that it works with both DNF4 as well as with DNF5.
@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@marusak marusak merged commit 41119be into rhinstaller:master Aug 20, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants