-
Notifications
You must be signed in to change notification settings - Fork 24
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
Removing dependencies for sysklogd #124
base: master
Are you sure you want to change the base?
Conversation
Since sysklogd is needed only for EL5, moving it with EL5 specifics
Removing sysklogd dependencies as RHEL 5 is now only in extended life support and CentOS 5 is end of life since 2017
@@ -187,7 +187,9 @@ | |||
$default_syslogd_options = '-m 0' | |||
$default_mod_imjournal = false | |||
$default_manage_devlog = false | |||
} | |||
# ensures that sysklogd is absent, which is needed on EL5 | |||
require '::sysklogd' |
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.
+1 - this should only be applied to EL5 and not other platforms
metadata.json
Outdated
@@ -65,7 +65,6 @@ | |||
} | |||
], | |||
"dependencies": [ | |||
{"name":"ghoneycutt/sysklogd","version_requirement":">= 1.0.0 < 2.0.0"}, |
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.
It is still a soft dependency and this is needed for the spec tests to work so it should stay. Also doesn't hurt anything by it being there, so I don't understand the motivation to remove it.
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.
It was to avoid an unecessary pull from the puppet forge. But if your need it for tests, it's a good reason to keep it
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.
Can you please remove the change to metadata.json
from your pull request and squash/rebase. The other change to only require sysklodg inside the EL5 case statement is still needed.
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.
Done (I think, I'm not used to github)
As sysklogd module is only useful for EL5 and this version is no longer actively developed, I suggest to drop the mandatory requirement to the module removing this package