Fix kernel_module for centos/redhat #1513
Conversation
Signed-off-by: Andrey Aleksandrov <[email protected]>
There was a problem hiding this comment.
Thank you @postgred for bringing up the change! Have you tested this on RedHat 6 and RedHat 7?
lib/resources/kernel_module.rb
Outdated
| lsmod_cmd = 'lsmod' | ||
| # special care for CentOS 5 and sudo | ||
| lsmod_cmd = '/sbin/lsmod' if inspec.os[:name] == 'centos' && inspec.os[:release].to_i == 5 | ||
| if inspec.os[:family] == 'redhat' |
There was a problem hiding this comment.
We should try to avoid using inspec.os[:family] . A better approach is to use inspec.os.redhat?
lib/resources/kernel_module.rb
Outdated
| @@ -37,7 +38,7 @@ def loaded? | |||
| end | |||
|
|
|||
| def version | |||
| if inspec.os[:name] == 'centos' && inspec.os[:release].to_i == 5 | |||
| if inspec.os[:family] == 'redhat' | |||
There was a problem hiding this comment.
This would resolve to true for any version of any rhel-based distro, so high chance it will either break for some of existing versions or it will break for some of future versions. I would suggest to keep this check, but only for distro versions in trouble.
There was a problem hiding this comment.
@Fodoj thank you for highlighting this issue. I assume this applies to RedHat 5 as well, therefore we may need to ask for
if inspec.os.redhat? && inspec.os[:release].to_i == 5
Signed-off-by: Andrey Aleksandrov <[email protected]>
|
@chris-rock Yeah, I have tested this on CentOS5-7 and RedHat6-7 |
lib/resources/kernel_module.rb
Outdated
| @@ -37,7 +38,7 @@ def loaded? | |||
| end | |||
|
|
|||
| def version | |||
| if inspec.os[:name] == 'centos' && inspec.os[:release].to_i == 5 | |||
| if inspec.os.family == 'redhat' | |||
There was a problem hiding this comment.
@postgred it is better to use inspec.os.redhat?. we try to avoid inspec.os.family. This is inherited from Serverspec. We are going to prefer the name or the helper method.
There was a problem hiding this comment.
Yes, If you don't want use os.family, we can use 'os.redhat? || os.name == 'fedora'` (I have found the problem on fedora too).
There was a problem hiding this comment.
@postgred Yes, that would be appreciated.
Signed-off-by: Andrey Aleksandrov <[email protected]>
|
@chris-rock I have removed os.family. Should I do something else? |
|
@Fodoj do you see any issues left? |
|
lgtm |
|
Thank you @postgred |
|
Looks great. Thanks @postgred! |
Hi! I fixed kernel_module resource for centos/redhat.
issue: #1507