grub_conf resource: fix menuentry detection

[jerryaldrichiii]

This does the following:

• Corrects Grub2 bug where last entry was always selected
• Adds support for specifying a Grub2 menu entry by name
• Adds support for using GRUB_DEFAULT=saved with Grub2
• Adds more Unit tests

This fixes #2333

[adamleff]

This looks like a good first step. Another potential option would be to use the parselet stuff that's used in the nginx_conf resource, but might be overkill here. I'd encourage you to at least take a look at it and understand how it works and decide which one is cleaner. What you have here is probably fine with a bit more of a defensive stance.

[adamleff]

What if .match doesn't match the line, and then capture_data is nil?

[jerryaldrichiii]

We should be fine since we don't make it to that bit unless line contains menuentry. That being said, I'm all for adding an exception to ensure the user gets a better error than a nil error.

[adamleff]

It's possible that a line can contain menuentry but still not pass your regex... unless you're willing to tell me you'd bet 3 months' salary that EVERY line in that file that has the word "menuentry" in it will ALWAYS match that regex. :)

[jerryaldrichiii]

Hah! Solid point. It would have to contain menuentry at the start of the line followed by at least 1 whitespace. I am not willing to take that bet though!

Let me know if failing the resource is appropriate here.

[adamleff]

Careful with this new helper method... this will raise SkipResource exceptions if the file isn't there or is empty or isn't readable. Are we guaranteed it will be there and with content?

[jerryaldrichiii]

Great catch! That is not a guarantee according to https://www.gnu.org/software/grub/manual/grub/html_node/Environment-block.html

I will fall back to the 0th entry the same way as if the saved_entry value is not up to date.

[adamleff]

@jerryaldrichiii can you please rebase on master so we can try to get travis fixed for this PR?

[jerryaldrichiii]

@adamleff I have rebased and pushed. 👍

[adamleff]

This is looking pretty good, @jerryaldrichiii. I still think we need a bit of defensive coding around the menuentry line and the regex parsing of it. I've left you a couple suggestions.

[adamleff]

I still think it's possible for capture_data to POSSIBLY return nil, and then this line will throw a "undefined method on NilClass" error.

If you really want to fail the whole resource if we can't parse the line, this should probably look like this:

if capture_data.nil? || capture_data.captures[0].nil?
  raise Inspec::Exceptions::ResourceFailed "Cannot extract menuentry name from line: #{line}"
end

entry['name'] = capture_data.captures[0]

You could also use the safe navigation operator to your advantage here as well:

entry['name'] = capture_data&.captures[0]
if entry['name'].nil?
  raise Inspec::Exceptions::ResourceFailed "Cannot extract menuentry name from line: #{line}"
end

[jerryaldrichiii]

I agree, better to raise anything but "undefined method for NilClass".

I went with your former suggestion. The safe navigation operator is neat, but it threw me for a loop the first time I saw it and I wouldn't want it to trip anyone else up later.

[adamleff]

If we're always grabbing the first, we could change the select to a find which always returns the first element where the block evals true

[jerryaldrichiii]

Neat! I didn't know that, will update.

[adamleff]

This is one of those places where I'd probably do this:

return menu_entries[default.to_i] unless default == 'saved'

grubenv_contents = ....

Anytime I see a single-line else or a really small else block, I'd try and short-circuit the method early and eliminate one level of indentation.

Not a blocker for this, just something to look out for in the future.

[jerryaldrichiii]

Great catch! I changed it because I believe that way to be much cleaner.

[arlimus]

I love the improvements in the detection @jerryaldrichiii thank you for adding this!!

@adamleff kudos for your great review as well