-
Notifications
You must be signed in to change notification settings - Fork 10
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 RedHat init script #43
Conversation
manifests/install.pp
Outdated
|
||
case $facts['os']['family'] { | ||
'RedHat': { | ||
if scanf($facts['os']['release']['major'], '%i')[0] <= 6 { |
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.
interesting to use scanf()
and not versioncmp()
. The latter is more common, but scanf
is cooler. 👍
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.
Could we go with versioncmp
though? :)
A single if
instead of a case
and an if
would probably be simpler too.
manifests/install.pp
Outdated
case $facts['os']['family'] { | ||
'RedHat': { | ||
if scanf($facts['os']['release']['major'], '%i')[0] <= 6 { | ||
file{'/etc/init.d/ferm': |
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.
could you create a new parameter like $manage_initfile
or something like that? The file isn't in the official rpm, so people might already have a custom script inplace that we overwrite. So we should only deploy the file, if a user explicitly requests it. Otherwise the next release needs to be a major one instead a minor one.
Hi @kBite, thanks for the PR. Can you probably fix the rubocop warnings by doing this:
|
spec/classes/ferm_spec.rb
Outdated
@@ -22,6 +22,9 @@ | |||
it { is_expected.to contain_file('/etc/ferm.d/chains') } | |||
it { is_expected.not_to contain_service('ferm') } | |||
it { is_expected.not_to contain_file('/etc/ferm.conf') } | |||
if facts[:os]['family'] == 'RedHat' and facts[:os]['release']['major'].to_i <= 6 |
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.
@kBite Please use &&
instead of and
to fix the RuboCop issue.
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.
@dhoppe Done. Please see my updated commits.
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.
Looks good. Thank you very much for your contribution.
479f8dd
to
a45c5af
Compare
Force pushed updated commits:
|
a45c5af
to
5574d83
Compare
OS like CentOS 6 are missing an init script for ferm. This PR ...