voxpupuli / puppet-yum

Puppet module for Yum
https://forge.puppet.com/puppet/yum
MIT License
16 stars 101 forks source link

Support dnf module management - Fix #310 #320

Closed EmersonPrado closed 6 months ago

EmersonPrado commented 1 year ago

Pull Request (PR) description

This PR adds a custom resource to freely enable/disable streams in DNF modules. The provider supports 4 different stream spec formats:

This Pull Request (PR) fixes the following issues

Fixes #310

WIP: though it passes the very limited spec tests and my manual tests, I pushed this in a hurry. Need to review later without tired eyes.

EmersonPrado commented 1 year ago

Double checked everything. I'm comfortable to have it reviewed

EmersonPrado commented 1 year ago

@jhoblitt Thanks for taking some time to review.

package dnfmodule provider has some issues with dealing with streams:

  1. A stream is a yum/dnf setting, not something (un)installable
  2. Any profile (un)install changes the stream, even if not told so
  3. Dealing with stream setting and profile installation in the same resource is messy - I resorted to Slack to understand the beast...
  4. It can be improved to fix 2 (in the provider) and 3 (in the docs), but me and the Slack dudes agreed 1 is a good reason not to do it - in fact, the agreeded proposal is to deprecate stream support in dnfmodule provider
jhoblitt commented 1 year ago

Could you provide a specific example in puppet code which does not work with the dnfmodule provider but does work with this new type/provider? dnfmodule does support profiles with the flavor parameters. Are you stating that Puppet By Perforce has decided to depreciate the the dnfmodule provider?

bastelfreak commented 1 year ago

@EmersonPrado can you add your explanation to the README.md?

EmersonPrado commented 1 year ago

@jhoblitt I'll do as @bastelfreak suggested. This might answer your questions, and the quite likely same ones from other people.

EmersonPrado commented 1 year ago

@jhoblitt @bastelfreak Done

EmersonPrado commented 1 year ago

@jhoblitt About this other question: "Are you stating that Puppet By Perforce has decided to depreciate the the dnfmodule provider?":

  1. Not Perforce, but other devs (maybe some from Perforce, I don't know), in Slack talks (I invited you to one of them, hope it gets to you)
  2. Not depreciate the dnfmodule provider, but move its stream support to a dedicated resource (and, later, complete its profiles support, but this is another issue)
bastelfreak commented 1 year ago

@EmersonPrado please update the REFERENCE.md with bundle exec rake strings:generate:reference (that's currently blocking the CI).

EmersonPrado commented 1 year ago

@bastelfreak Done. Still some errors, but I guess they're unrelated to the PR.

binford2k commented 1 year ago

Could you provide a specific example in puppet code which does not work with the dnfmodule provider but does work with this new type/provider? dnfmodule does support profiles with the flavor parameters. Are you stating that Puppet By Perforce has decided to depreciate the the dnfmodule provider?

No, the dnfmodule provider is not on the chopping block. What I believe Emerson is trying to do is clarify intent and also to provide a more flexible way to configure the multitude that is DNF.

Xazziri commented 6 months ago

Is there any change of this PR being merged? This looks like the perfect solution for a problem managing appstream modules I'm having...

bastelfreak commented 6 months ago

@Xazziri the failing tests need to be fixed before we can merge this.

EmersonPrado commented 6 months ago

@Xazziri the failing tests need to be fixed before we can merge this.

The previous state of these tests were some failure apparently not related to the changes, but now there seems to be quite more failures than before. Unfortunately, now the tests just say "The logs for this run have expired and are no longer available". Guess I need to provoke another run to get current status.

EmersonPrado commented 6 months ago

@bastelfreak Good news! Guess someone fixed something upstream, and the tests now pass! @jhoblitt Could you pls review this PR now?

bastelfreak commented 6 months ago

@EmersonPrado thanks for the work! Can you please rebase and remove commit 555306209c3216de00aebe5d1f01971824b67b8e and 023df4179d048e5731189907e366588f75f1088d?

EmersonPrado commented 6 months ago

@EmersonPrado thanks for the work! Can you please rebase and remove commit 5553062 and 023df41?

Done!