Fix the broken check-megraid-sas-status script in 2.0.1

[jothikanth]

Pull Request Checklist

Is this in reference to an existing issue?

General

• Update Changelog following the conventions laid out here

• Update README with any necessary configuration snippets

• Binstubs are created if needed

• RuboCop passes

• Existing tests pass

New Plugins

• Tests

• Add the plugin to the README

• Does it have a complete header as outlined here

Purpose

Recent version 2.0.1 has a syntax issue in check-megaraid-sas-status.

Known Compatibility Issues

[majormoses]

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

[majormoses]

Hmm I don't understand why that would fix it as we don't assign it so we can't use the matchdata. Can you please include a testing artifact which could be a simple gist showing the before and after the fix is applied. Also if I could see the output of the requested debug info I might be able to more easily spot the issue.

[majormoses]

I am having a hard time understanding why this does not work since we don't assign the returned match data anyways. From my tests it seems to have no effect.

$ irb
# assign vars
irb(main):001:0> a = Regexp.new('State: Optimal')
=> /State: Optimal/
irb(main):002:0> b = 'State: Optimal foo bar'
=> "State: Optimal foo bar"

# test using match?
irb(main):003:0> unless a.match?(b)
irb(main):004:1> msg = sprintf '%svirtual drive %d: %s ', error, i, b[/State\s*:\s*.*/].split(':')[1]
irb(main):005:1> p msg
irb(main):006:1> end
=> nil

# testing using match
irb(main):007:0> unless a.match(b)
irb(main):008:1> msg = sprintf '%svirtual drive %d: %s ', error, i, b[/State\s*:\s*.*/].split(':')[1]
irb(main):009:1> p msg
irb(main):010:1> end
=> nil

So I can better understand can you please include the output of adding p stdout: #{stdout}, class: #{stdout.class} before and after the unless?

[crscybr]

Does the following output with stdout before the unless suffice?
https://gist.github.com/crscybr/3aca2c9d0e61457581ad035ca91b9dc9

[majormoses]

It does actually help, I think I understand what is happening now, just to confirm can you verify if you are using a ruby that is < 2.4.0? The function match? was added in ruby 2.4 so that explains the error. I was under the impression that it was silently passing when it should have failed or failing when it should have passed. As this was an uncaught error I did not look to see if it was not available in certain versions of ruby. In addition to this script there are others that we should fix up. Any chance you would be willing to submit a PR to fix those up as well? If not I can maybe get to it tomorrow.

[majormoses]

This is why including a before and after run is critical for bug fixes as sometimes just by looking at the actual error I can quickly validate the fix much quicker.

[crscybr]

Perhaps the version info is relevant. The test above also fails on my system using sensu-0.26.5-2.x86_64
% /opt/sensu/embedded/bin/ruby -v
ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-linux]

irb(main):001:0> a = Regexp.new('State: Optimal')
=> /State: Optimal/
irb(main):002:0> b = 'State: Optimal foo bar'
=> "State: Optimal foo bar"
irb(main):003:0> unless a.match?(b)
irb(main):004:1> msg = sprintf '%svirtual drive %d: %s ', error, i, b[/State\s*:\s*.*/].split(':')[1]
irb(main):005:1> p msg
irb(main):006:1> end
NoMethodError: undefined method `match?' for /State: Optimal/:Regexp
Did you mean?  match
	from (irb):3
	from /opt/sensu/embedded/bin/irb:11:in `<main>'
irb(main):007:0>

[majormoses]

Yup that confirms it, this broke on ruby versions < 2.4.0.

[robertely]

Pardon my ignorance but would something like unless stdout =~ /State\s*:\s*Optimal/ work more broadly ?

[majormoses]

Sorry if my replies are erratic, I am on vacation this week.

Correct that should work based on my understanding of what each of the following does:

• =~ // returns the index of the index (integer) of the first match in the string otherwise returns nil if no match is found. Updates global var $~.
• .match(//) returns the a MatchData object which contains the matching data and useful metadata such as capture groups otherwise nil if no match is found. Updates global var $~.
• .match?(//) which was introduced in 2.4 returns a (true|false) and does not update any global vars such as $~

Since we don't need anything other than knowing if the data matches in this case we should use =~ for the time being to keep fix the bug that this affects in ruby < 2.4 setups. We will probably change that to match?(//) at some point when we drop ruby < 2.4 support as it is clearer in its intent for a ruby novice. Previously .match(//) was used and while we could restore it I think it would be best to fall back to =~ as this is closer to the .match?(//) implementation and therefore is more performant.

Sources:

• https://ruby-doc.org/core-2.4.1/doc/regexp_rdoc.html
• https://ruby-doc.org/core-2.4.1/Regexp.html#method-i-match-3F

[majormoses]

@robertely any chance you plan on coming back to this?