From 4728af6c0027d48165565313f74ae31e9de17798 Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Fri, 6 Nov 2015 09:16:52 -0800 Subject: [PATCH] add checks for correct use of use_inline_resources - recommend use_inline_resource in all 'LWRPBase' providers - catch the use of use_inline_resources when users are also declaring `def action_whatever` instead of `action :whatever` which defeats use_inline_resources --- ...vider_without_use_inline_resources.feature | 20 +++++ ...ibrary_provider_bad_action_methods.feature | 25 ++++++ ...vider_without_use_inline_resources.feature | 15 ++++ ...r_lwrp_provider_bad_action_methods.feature | 25 ++++++ features/step_definitions/cookbook_steps.rb | 82 ++++++++++++++++++- features/support/command_helpers.rb | 4 + lib/foodcritic/rules.rb | 41 ++++++++++ 7 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 features/057_check_for_library_provider_without_use_inline_resources.feature create mode 100644 features/058_check_for_library_provider_bad_action_methods.feature create mode 100644 features/059_check_for_lwrp_provider_without_use_inline_resources.feature create mode 100644 features/060_check_for_lwrp_provider_bad_action_methods.feature diff --git a/features/057_check_for_library_provider_without_use_inline_resources.feature b/features/057_check_for_library_provider_without_use_inline_resources.feature new file mode 100644 index 00000000..ff472855 --- /dev/null +++ b/features/057_check_for_library_provider_without_use_inline_resources.feature @@ -0,0 +1,20 @@ +Feature: Check for library providers that do not declare use_inline_resources + + In order to ensure that notifications happen correctly + As a cookbook provider author + I want to always use_inline_resources + + Scenario: Library provider with use_inline_resources + Given a cookbook that contains a library provider with use_inline_resources + When I check the cookbook + Then the library provider without use_inline_resources warning 057 should not be displayed against the libraries file + + Scenario: Library provider without use_inline_resources + Given a cookbook that contains a library provider without use_inline_resources + When I check the cookbook + Then the library provider without use_inline_resources warning 057 should be displayed against the libraries file on line 11 + + Scenario: Library file without use_inline_resources + Given a cookbook that contains a library resource + When I check the cookbook + Then the library provider without use_inline_resources warning 057 should not be displayed against the libraries file diff --git a/features/058_check_for_library_provider_bad_action_methods.feature b/features/058_check_for_library_provider_bad_action_methods.feature new file mode 100644 index 00000000..3debbcc3 --- /dev/null +++ b/features/058_check_for_library_provider_bad_action_methods.feature @@ -0,0 +1,25 @@ +Feature: Check for library providers that declare use_inline_resources and declare action_ methods + + In order to ensure that notifications happen correctly + As a cookbook provider author + I want to always use_inline_resources + + Scenario: Library provider with use_inline_resources and bad action_create + Given a cookbook that contains a library provider with use_inline_resources and uses def action_create + When I check the cookbook + Then the library provider without use_inline_resources and bad action_create warning 058 should be displayed against the libraries file on line 11 + + Scenario: Library provider without use_inline_resources and (okay) action_create + Given a cookbook that contains a library provider without use_inline_resources and uses def action_create + When I check the cookbook + Then the library provider without use_inline_resources and bad action_create warning 058 should not be displayed against the libraries file + + Scenario: Library provider without use_inline_resources + Given a cookbook that contains a library provider without use_inline_resources + When I check the cookbook + Then the library provider without use_inline_resources and bad action_create warning 058 should not be displayed against the libraries file + + Scenario: Library provider with use_inline_resources + Given a cookbook that contains a library provider with use_inline_resources + When I check the cookbook + Then the library provider without use_inline_resources and bad action_create warning 058 should not be displayed against the libraries file diff --git a/features/059_check_for_lwrp_provider_without_use_inline_resources.feature b/features/059_check_for_lwrp_provider_without_use_inline_resources.feature new file mode 100644 index 00000000..4269f13f --- /dev/null +++ b/features/059_check_for_lwrp_provider_without_use_inline_resources.feature @@ -0,0 +1,15 @@ +Feature: Check for LWRP providers that do not declare use_inline_resources + + In order to ensure that notifications happen correctly + As a cookbook provider author + I want to always use_inline_resources + + Scenario: LWRP provider with use_inline_resources + Given a cookbook that contains a LWRP provider with use_inline_resources + When I check the cookbook + Then the LWRP provider without use_inline_resources warning 059 should not be displayed against the provider file + + Scenario: LWRP provider without use_inline_resources + Given a cookbook that contains a LWRP provider without use_inline_resources + When I check the cookbook + Then the LWRP provider without use_inline_resources warning 059 should be displayed against the provider file diff --git a/features/060_check_for_lwrp_provider_bad_action_methods.feature b/features/060_check_for_lwrp_provider_bad_action_methods.feature new file mode 100644 index 00000000..54effd5b --- /dev/null +++ b/features/060_check_for_lwrp_provider_bad_action_methods.feature @@ -0,0 +1,25 @@ +Feature: Check for LWRP providers that declare use_inline_resources and declare action_ methods + + In order to ensure that notifications happen correctly + As a cookbook provider author + I want to always use_inline_resources + + Scenario: LWRP provider with use_inline_resources and bad action_create + Given a cookbook that contains a LWRP provider with use_inline_resources and uses def action_create + When I check the cookbook + Then the LWRP provider without use_inline_resources and bad action_create warning 060 should be displayed against the provider file on line 3 + + Scenario: LWRP provider without use_inline_resources + Given a cookbook that contains a LWRP provider without use_inline_resources + When I check the cookbook + Then the LWRP provider without use_inline_resources and bad action_create warning 060 should not be displayed against the provider file + + Scenario: LWRP provider without use_inline_resources and (okay) action_create + Given a cookbook that contains a LWRP provider without use_inline_resources and uses def action_create + When I check the cookbook + Then the LWRP provider without use_inline_resources and bad action_create warning 060 should not be displayed against the provider file + + Scenario: LWRP provider with use_inline_resources + Given a cookbook that contains a LWRP provider with use_inline_resources + When I check the cookbook + Then the LWRP provider without use_inline_resources and bad action_create warning 060 should not be displayed against the provider file diff --git a/features/step_definitions/cookbook_steps.rb b/features/step_definitions/cookbook_steps.rb index a862cee7..74bc2432 100644 --- a/features/step_definitions/cookbook_steps.rb +++ b/features/step_definitions/cookbook_steps.rb @@ -2083,7 +2083,7 @@ def search(bag_name, query=nil, sort=nil, start=0, rows=1000, &block) expect_warning code, {:expect_warning => should_not.nil?} end -Then /^the (?:[a-zA-Z \-_]+) warning ([0-9]+) should (not )?be displayed(?: against the (attributes|libraries|definition|metadata|provider|resource|README.md|README.rdoc) file)?( below)?$/ do |code, no_display, file, warning_only| +Then /^the (?:[a-zA-Z \-_]+) warning ([0-9]+) should (not )?be displayed(?: against the (attributes|libraries|definition|metadata|provider|resource|README.md|README.rdoc) file)?( below)?(?: on line ([0-9]+))?$/ do |code, no_display, file, warning_only, line| options = {} options[:expect_warning] = no_display != 'not ' unless file.nil? @@ -2095,6 +2095,7 @@ def search(bag_name, query=nil, sort=nil, start=0, rows=1000, &block) end options[:line] = 3 if code == '018' and options[:expect_warning] options[:line] = 2 if ['021', '022'].include?(code) + options[:line] = line unless line.nil? options[:warning_only] = ! warning_only.nil? expect_warning("FC#{code}", options) end @@ -2331,3 +2332,82 @@ def search(bag_name, query=nil, sort=nil, start=0, rows=1000, &block) Then /^the metadata using recommends warning 053 should be (shown|not shown) against the metadata file$/ do |show_warning| expect_warning('FC053', :file => "metadata.rb", :line => 2, :expect_warning => show_warning == 'shown') end + +Given /^a cookbook that contains a LWRP provider (with|without) use_inline_resources( and uses def action_create)?$/ do |with_use_inline_resources, uses_def| + write_resource("site", %q{ + actions :create + attribute :name, :kind_of => String, :name_attribute => true + }) + provider_file = "" + if with_use_inline_resources == 'with' + provider_file += %q{ + use_inline_resources + } + end + if uses_def + provider_file += %q{ + def action_create + } + else + provider_file += %q{ + action :create do + } + end + provider_file += %q{ + file "/tmp/foo.txt" + end + } + write_provider("site", provider_file) +end + +Given /^a cookbook that contains a library provider (with|without) use_inline_resources( and uses def action_create)?$/ do |with_use_inline_resources, uses_def| + library_file = %q{ + class MyResources + class Site < Chef::Resource::LWRPBase + provides :site + resource_name :site + actions :create + attribute :name, :kind_of => String, :name_attribute => true + end + end + + class MyProviders + class Site < Chef::Provider::LWRPBase + provides :site + } + if with_use_inline_resources == 'with' + library_file += %q{ + use_inline_resources + } + end + if uses_def + library_file += %q{ + def action_create + } + else + library_file += %q{ + action :create do + } + end + library_file += %q{ + file "/tmp/foo.txt" + end + end + end + } + write_library('lib', library_file) +end + +Given /^a cookbook that contains a library resource$/ do + library_file = %q{ + class MyResources + class Site < Chef::Resource::LWRPBase + provides :site + resource_name :site + actions :create + attribute :name, :kind_of => String, :name_attribute => true + end + end + } + write_library('lib', library_file) +end diff --git a/features/support/command_helpers.rb b/features/support/command_helpers.rb index 6be636e6..42f7350c 100644 --- a/features/support/command_helpers.rb +++ b/features/support/command_helpers.rb @@ -67,6 +67,10 @@ def assertions # FC054 was yanked and is considered reserved, do not reuse it 'FC055' => 'Ensure maintainer is set in metadata', 'FC056' => 'Ensure maintainer_email is set in metadata', + 'FC057' => 'Library provider does not declare use_inline_resources', + 'FC058' => 'Library provider declares use_inline_resources and declares #action_ methods', + 'FC059' => 'LWRP provider does not declare use_inline_resources', + 'FC060' => 'LWRP provider declares use_inline_resources and declares #action_ methods', 'FCTEST001' => 'Test Rule' } diff --git a/lib/foodcritic/rules.rb b/lib/foodcritic/rules.rb index bfff4411..3a3e444b 100644 --- a/lib/foodcritic/rules.rb +++ b/lib/foodcritic/rules.rb @@ -785,3 +785,44 @@ def invalid_name(ast) [file_match(filename)] unless field(ast, 'maintainer_email').any? end end + +rule 'FC057', 'Library provider does not declare use_inline_resources' do + tags %w(correctness) + library do |ast, filename| + ast.xpath('//const_path_ref/const[@value="LWRPBase"]/..//const[@value="Provider"]/../../..').select do |x| + x.xpath('//*[self::vcall or self::var_ref]/ident[@value="use_inline_resources"]').empty? + end + end +end + +rule 'FC058', 'Library provider declares use_inline_resources and declares #action_ methods' do + tags %w(correctness) + library do |ast, filename| + ast.xpath('//const_path_ref/const[@value="LWRPBase"]/..//const[@value="Provider"]/../../..').select do |x| + x.xpath('//*[self::vcall or self::var_ref]/ident[@value="use_inline_resources"]') && + x.xpath(%Q(//def[ident[contains(@value, 'action_')]])) + end + end +end + +rule 'FC059', 'LWRP provider does not declare use_inline_resources' do + tags %w(correctness) + provider do |ast, filename| + use_inline_resources = !ast.xpath('//*[self::vcall or self::var_ref]/ident + [@value="use_inline_resources"]').empty? + unless use_inline_resources + [file_match(filename)] + end + end +end + +rule 'FC060', 'LWRP provider declares use_inline_resources and declares #action_ methods' do + tags %w(correctness) + provider do |ast, filename| + use_inline_resources = !ast.xpath('//*[self::vcall or self::var_ref]/ident + [@value="use_inline_resources"]').empty? + if use_inline_resources + ast.xpath(%Q(//def[ident[contains(@value, 'action_')]])) + end + end +end