0

I have a chef resource and I just added linting with cookstyle to the cookbook. I have this line in the cookbook:

existing_value = key.to_s.split('.').inject(node.elasticsearch) { |result, attr| result[attr] } rescue nil if existing_value.nil?

It's causing the linter to error. I tried a couple of things, but I'm not sure how to say the same thing and pass the lint test. 

 Style/RescueModifier: Avoid using rescue in its modifier form

If I remove the rescue statement entirely, it causes this exception: 

Chef::Mixin::Template::TemplateError (undefined method `[]' for nil:NilClass) on line #52:
    ...
    52: <%= print_value 'action.destructive_requires_name' -%>        
    ...
numb3rs1x
  • 4,673
  • 5
  • 31
  • 44
  • 1
    That's a catch-all `rescue` which is really bad in practice. Can you remove the `rescue nil` part and still have it working? If not, what exceptions does it raise? – tadman Apr 10 '17 at 22:53
  • What is the value of `key`? Please read "[mcve]" and the linked page. The error is telling us that `result` is nil. – the Tin Man Apr 10 '17 at 23:42
  • The value of `key`, line 52 `action.destructive_requires_name` in this example is `nil` which is in the output of the exception that I posted. – numb3rs1x Apr 10 '17 at 23:45
  • One minor issue, please don't use `chefstyle`, that is for Chef itself. You want to use `cookstyle` which is for cookbooks. – coderanger Apr 11 '17 at 00:10
  • it's actually cookstyle. my mistake. – numb3rs1x Apr 25 '17 at 21:42

2 Answers2

1

So we actually have a method for this if you're on a mildly recent Chef version. You can do this instead:

existing_value ||= node.read("elasticsearch.#{key}")

And we'll take care of the rest.

coderanger
  • 52,400
  • 4
  • 52
  • 75
0

Although #coderanger's solution is better, the linter wants you to use the long rescue form

begin
  existing_value = key.to_s.split('.').inject(node.elasticsearch) { |result, attr| result[attr] } 
rescue 
  nil
end if existing_value.nil?

or usually considered better

if existing_value.nil?
  begin
    existing_value = key.to_s.split('.').inject(node.elasticsearch) { |result, attr| result[attr] } 
  rescue 
    nil
  end
end

I would also change rescue to the more specific rescue Chef::Mixin::Template::TemplateError, rescuing all exceptions is probably not what you want (i.e. you wouldn't want to hide a real exception)

coderanger
  • 52,400
  • 4
  • 52
  • 75
Marc Rohloff
  • 1,332
  • 7
  • 8
  • The actual exception is something different, we just wrap all errors coming out of Erb so they get displayed with some special logic. – coderanger Apr 11 '17 at 07:54
  • There are countless articles explaining why this is a bad thing. For example you shouldn't catch SystemExit or OutOfMemory errors, because there are other reasons those are thrown. You should at least restrict it to subclasses of `StandardError` – Marc Rohloff Apr 11 '17 at 15:32