-
Notifications
You must be signed in to change notification settings - Fork 681
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
Version method for kernel_module #1435
Conversation
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.
Thank you @postgred for this great addition. I added ideas for minor improvements.
Right now, rubocop is failing with:
Offenses:
lib/resources/kernel_module.rb:46:42: C: Use delete instead of gsub.
cmd.exit_status.zero? ? cmd.stdout.gsub("\n", '') : false
^^^^^^^^^^^^^^
Could you please sign-off your work, too. Instructions are available here: https://github.com/chef/inspec/blob/master/CONTRIBUTING.md#developer-certification-of-origin-dco
# default modinfo command | ||
modinfo_cmd = "modinfo -F version #{@module}" | ||
# command for CentOS 5 and sudo | ||
modinfo_cmd = "/sbin/modinfo -F version #{@module}" if inspec.os[:name] == 'centos' && inspec.os[:release].to_i == 5 |
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.
maybe:
if inspec.os.name == 'centos' && inspec.os.release.to_i == 5
...
else
modinfo_cmd = "modinfo -F version #{@module}"
end
modinfo_cmd = "/sbin/modinfo -F version #{@module}" if inspec.os[:name] == 'centos' && inspec.os[:release].to_i == 5 | ||
|
||
cmd = inspec.command(modinfo_cmd) | ||
cmd.exit_status.zero? ? cmd.stdout.gsub("\n", '') : false |
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 may be better to return nil
, since we could not determine a version
Signed-off-by: Andrey Aleksandrov <[email protected]>
Signed-off-by: Andrey Aleksandrov <[email protected]>
@postgred Thank you for the quick improvements. The implementation looks great. I was just thinking, we should add this to our integration tests as well, to ensure it works across linux systems. The test is located here: https://github.com/chef/inspec/blob/master/test/integration/default/kernel_module_spec.rb#L21-L23 This will be executed with every travis run. See https://github.com/chef/inspec/blob/master/.travis.yml#L28-L55 |
@chris-rock We can't add this to integration tests because modinfo can't work in docker. Example:
|
@postgred Agreed. Thank you. |
Hi! This version method for kernel_module.
issue: #993