Foodcritic / foodcritic

Lint tool for Chef cookbooks.
http://foodcritic.io
MIT License
408 stars 153 forks source link

FC022 - Foodcritic complains about resources defined in a loop, but resource not defined in a loop #632

Open jayhendren opened 7 years ago

jayhendren commented 7 years ago
[birdsnest ~/local/data/sis/git/cookbooks/cub_jenkins](polishing|✚1)[I]% foodcritic .
Checking 12 files
.......x....
FC022: Resource condition within loop may not behave as expected: ./recipes/master.rb:17

And here is recipes/master.rb up to the resource defined on line 17 (ruby_block[trigger jenkins ...]):

include_recipe 'cub_java'
include_recipe 'jenkins::master'
include_recipe 'cub_jenkins::proxy'

node['cub_jenkins']['plugins'].each do |name, ver|
  jenkins_plugin name do
    version ver
    # don't do dependency resolution on plugins. see attributes/master.rb and
    # https://github.com/chef-cookbooks/jenkins/issues/534
    install_deps false
  end
end

# restart jenkins now if any of above plugins have been updated.
# keypair credentials below depends on plugins being loaded
# which won't happen until jenkins is restarted.
ruby_block 'trigger jenkins restart to enable plugins' do
  block {}
  notifies :restart, 'service[jenkins]', :immediately
  only_if do
    node['cub_jenkins']['plugins'].map do |name, _version|
      resources("jenkins_plugin[#{name}]").updated_by_last_action?
    end.any?
  end
end

Foodcritic is complaining that the ruby_block resource is defined with a fixed name in a loop, but it's not defined in a loop. The previously defined resources are in a loop, which seems to trigger the issue. if I comment out either the node['cub_jenkins']['plugins'].each loop or the ruby_block resource, then foodcritic stops complaining.

[birdsnest ~/local/data/sis/git/cookbooks/cub_jenkins](polishing|✚1)[I]% foodcritic --version
foodcritic 10.2.2
coderanger commented 7 years ago

We can probably just deprecate this rule since in 13+ there is no error (other than maybe some confusion around notifications).

tas50 commented 7 years ago

I'm ok with nuking this one if someone (@coderanger) wants to cut a PR that explains why this isn't an issue anymore and why we don't need it.

WalterS commented 6 years ago

I get the same for

package 'python-xml' do
  action :install
  only_if { node['os_family'] == 'suse' && check_package(new_package).empty? } 
end

with foodcritic 12.3.0