-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Changes RedHat repo to use the EPEL module #603
Conversation
7fc97c5
to
6adb075
Compare
+1 |
@petems I know this is simply following the pattern, but this is a soft dependency, no? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good move to use logic of the existing epel module.
It could be considered as a breaking change because it introduces a new dependeny and it may leave unmanaged yumrepos config behind.
I don't like the use of |
I don't like the use of ```require```. Can we avoid it?
Perhaps also consider ```$repo_class``` as a variable
Or just make it completly optional by requiring users to include a repo subclass if they require it? Somewhat Roles/profiles style.
|
I was trying to match the original behaviour as much as possible, which is to default to have the upstream class enabled. |
6adb075
to
5596b86
Compare
@petems can you rebase? Then rubocop should be happy again. |
5596b86
to
c7ba015
Compare
@bastelfreak Done! 👍 |
hrm damn, the recently merged cuda plugin uses the yum module as well. https://travis-ci.org/voxpupuli/puppet-collectd/jobs/200865588#L3647-L3654 |
* Yum::Install to install the RPM is a bit more flakey * EPEL module is pretty ubiquitous, not that big of a deal to add it
c7ba015
to
93d6f3e
Compare
@bastelfreak Ah, fixed. That's repeated code that could probably be DRY-ed up, but I'll leave that to a seperate PR! 👍 |
thanks! |
Installing the RPM is clunky and 99% of people use the EPEL module