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

@marusak marusak force-pushed the dnf5-config-manager branch from 9033e4e to 673a27f Compare May 20, 2024 11:29
@marusak marusak requested a review from jkonecny12 May 21, 2024 06:14
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.

@github-actions github-actions bot added the stale label Jul 21, 2024
@marusak marusak removed the stale label Aug 2, 2024
@marusak marusak force-pushed the dnf5-config-manager branch from 673a27f to b4bae77 Compare August 14, 2024 11:31
@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

@marusak marusak force-pushed the dnf5-config-manager branch from b4bae77 to a0903a7 Compare August 14, 2024 13:02
@marusak marusak force-pushed the dnf5-config-manager branch from a0903a7 to 6f39cb0 Compare August 15, 2024 06:35
@marusak marusak force-pushed the dnf5-config-manager branch from 6f39cb0 to 5b27e24 Compare August 15, 2024 07:22
@marusak marusak requested a review from jkonecny12 August 15, 2024 08:25
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

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 marusak force-pushed the dnf5-config-manager branch from 5b27e24 to a83757f Compare August 15, 2024 13:55
@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