Foodcritic / foodcritic

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

FC120 issue ("Do not set the name property directly on a resource") #775

Closed rmoriz closed 6 years ago

rmoriz commented 6 years ago

iirc this usage is quite common to circumvent overwritten resources, e.g.

# recipe.rb
service 'reload httpd' do
   name 'httpd'
   action :reload
end

[...]

service 'stop httpd in case of X' do
   name 'httpd'
   action :stop
   only_if "..."
end
tas50 commented 6 years ago

What you're doing there is exactly what we're trying to catch. You're setting the resource name to 'reload httpd' and then changing it to 'httpd' right afterwards. Instead you want to do this

service 'reload httpd' do
   service_name 'httpd'
   action :reload
end

service_name is the name property and is the proper way to set the name of the service without messing with Chef's internal resource collection by changing the whole resource name.

ariksu commented 5 years ago

Well, what should we do on cookbooks where the name is used as a key part?

https://supermarket.chef.io/cookbooks/aws


aws_ssm_parameter_store "create encrypted test kitchen record" do
  name '/testkitchen/EncryptedStringCustomKey'
  description 'Test Kitchen Encrypted Parameter - Custom'
  value 'Encrypted Test Kitchen Custom'
  type 'SecureString'
  key_id '5d888999-5fca-3c71-9929-014a529236e1'
  action :create
  aws_access_key node['aws_test']['key_id']
  aws_secret_access_key node['aws_test']['access_key']
 end
granite-cloud commented 5 years ago

Yes I just ran into the aws_ssm_parameter_store scenario above and since I use this frequently in a couple cookbooks, what is it that we should do? It seems really horrible to have to run around and do the # ~ FC120 on every resource that this is a false positive.

I am trying to do things the right way and implement proper lint testing but I keep running into these one offs in foodcritic and rubucop that there are no choices. Maybe I am missing something with the aws_ssm_parameter_store issue , so does anyone have any pointers there?

thanks

pmacdougall commented 5 years ago

AWS aws_route53_record and aws_route53_zone are another cases where name is a actually a required property.

tas50 commented 5 years ago

Those are bugs that need to get fixed