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

add params rhel_satellite #1032

Closed
wants to merge 1 commit into from
Closed

Conversation

flepoutre
Copy link

Hello,
The purpose of this PR is to not install the centos-release-rabbitmq-38 package if we used RHEL satellite.
Can you please take it into account ?
Thanks.

Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

@flepoutre: Thanks for this. I had an idea this might cause a problem, but because there's no working CI setup for acceptance tests that works for this module on RHEL (there is an image, but IIRC, there are non-available required packages that make practically using it for Puppet tests difficult), it was hard to know for sure, and also didn't know what the desired behavior would be in the case of RHEL.

High level, I think this is about the right fix, but I think we should do a combination of:

  • A bit more magic internally
    and / or
  • Rename the parameter to something more consistent and generic (like enable_centos_release (bool)).

I think we're trying to do less of "data in modules", so the fix of setting the param default to true when the exact OS is CentOS, and false otherwise might not work.

But we can probably use a combination of facts and defaults to get this to a somewhat better place.

You'll also need to update the generated docs, and possibly adjust one or two spec tests.

@jay7x
Copy link
Member

jay7x commented Dec 11, 2024

Please update REFERENCE.md with bundle exec rake strings:generate:reference.

Also, parameter name is confusing. I'd say the whole logic there is confusing. From my point of view, it should be under $repos_ensure (which, in turn, should be named $manage_repo for consistency, but it's different topic). I'm not that good in RHELs, but I guess that package in question installs the repo actually. Forget this, if I'm wrong. Though the parameter should be renamed in any case.

wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request Dec 11, 2024
Add an `enable_centos_release` parameter, defaulting to `false`, except
on CentOS, where it defaults to `true. If enabled on any OS in the
RedHat family when `repos_ensure` is also disabled, it controls adding
the `centos-release-rabbitmq` package (currently
`centos-release-rabbitmq-38`)

Fixes voxpupuli#1033
Closes voxpupuli#1032
@wyardley
Copy link
Contributor

This is a kind of wonky one. I implemented something that I think will resolve this in #1034 – hope that’s ok?

@flepoutre
Copy link
Author

@wyardley Perfect many thanks for => #1034 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants