From 9e1c19c596cd2ae207e47920a128370f993931a4 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 22 Feb 2016 13:35:31 -0800 Subject: [PATCH 1/4] Potentially fix FC033 false positives To be honest I'm not entirely sure what was going on here before, but it certainly seemed complex for what we're trying to do. All we need to do is see if anything in the templates match the sources defined template file. We have a method for grabbing all the templates and it does the right thing out of the box returning templates that are in the root of the templates dir (vs. in default or what not). I tested this against the working cookbook and then put in a bogus template source and it triggered there as expected. --- lib/foodcritic/rules.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/foodcritic/rules.rb b/lib/foodcritic/rules.rb index a7aa16e9..396865e3 100644 --- a/lib/foodcritic/rules.rb +++ b/lib/foodcritic/rules.rb @@ -498,14 +498,8 @@ def pry_bindings(ast) end.reject do |resource| resource[:file].respond_to?(:xpath) end.select do |resource| - template_paths(filename).none? do |path| - relative_path = [] - Pathname.new(path).ascend do |template_path| - relative_path << template_path.basename - break if template_path.dirname.dirname.basename.to_s == "templates" - end - File.join(relative_path.reverse) == resource[:file] - end + # return true if none of the basenames of the templates match the filename in the resource + ! template_paths(filename).any? { |path| File.basename(path) == resource[:file] } end.map { |resource| resource[:resource] } end end From 9fe7a255a3219346510098038be78dab8fc633d0 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 22 Feb 2016 23:25:54 -0800 Subject: [PATCH 2/4] Support template sources that are arrays --- lib/foodcritic/api.rb | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/foodcritic/api.rb b/lib/foodcritic/api.rb index 210a3ffe..416b6ad2 100644 --- a/lib/foodcritic/api.rb +++ b/lib/foodcritic/api.rb @@ -359,7 +359,26 @@ def supported_platforms(ast) end.sort { |a, b| a[:platform] <=> b[:platform] } end - # Template filename + # Returns template source filenames if specified or implied by resource name + def template_files(resource) + if resource['source'] + if resource['source'].respond_to?(:xpath) # source is an array + resource['source'].xpath('//array//tstring_content/@value').map {|x| x.to_s} + else # source is a string + [resource['source']] + end + elsif resource[:name] + if resource[:name].respond_to?(:xpath) + [resource[:name]] + else + ["#{File.basename(resource[:name])}.erb"] + end + end + end + + # return template source filename + # This is deprecated as it returns a string only which isn't compatible + # with arrays of souce files in Chef 12 def template_file(resource) if resource["source"] resource["source"] @@ -388,7 +407,7 @@ def templates_included(all_templates, template_path, depth = 1) end.flatten.uniq.compact end - # Templates in the current cookbook + # Templates source files in the current cookbook def template_paths(recipe_path) Dir.glob(Pathname.new(recipe_path).dirname.dirname + "templates" + "**/*", File::FNM_DOTMATCH).select do |path| From f5f0c9df1047e9cfa8e8302066a824e829722074 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 22 Feb 2016 23:28:58 -0800 Subject: [PATCH 3/4] Update name and tests Signed-off-by: Tim Smith --- .../033_check_for_missing_template.feature | 41 +++++++++---------- spec/foodcritic/api_spec.rb | 1 + 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/features/033_check_for_missing_template.feature b/features/033_check_for_missing_template.feature index bb0ec2e4..9d67b5aa 100644 --- a/features/033_check_for_missing_template.feature +++ b/features/033_check_for_missing_template.feature @@ -1,4 +1,4 @@ -Feature: Check for missing template +Feature: Check for missing template source file(s) In order to ensure the Chef run is successful As a developer @@ -7,25 +7,22 @@ Feature: Check for missing template Scenario Outline: Template types Given a cookbook recipe that When I check the cookbook - Then the missing template warning 033 + Then the warning 033 should be Examples: - | template_type | warning | - | defines a template where both the name and source are complex expressions | should not be displayed | - | defines a template where name and source are both simple expressions | should not be displayed | - | defines a template where name is a complex expression | should not be displayed | - | infers a template with an expression | should not be displayed | - | refers to a hidden template | should not be displayed | - | refers to a local template | should not be displayed | - | refers to a missing template | should be displayed | - | refers to a template in a subdirectory | should not be displayed | - | refers to a template | should not be displayed | - | refers to a template with an expression | should not be displayed | - | refers to a template without an erb extension | should not be displayed | - | uses a missing inferred template | should be displayed | - | uses an inferred template | should not be displayed | - | uses a template from another cookbook | should not be displayed | - - Scenario: Template within deploy resource - Given a cookbook recipe with a deploy resource that contains a template resource - When I check the cookbook - Then the missing template warning 033 should not be displayed against the template + | template_type | warning | + | defines a template where both the name and source are complex expressions | valid | + | defines a template where name and source are both simple expressions | valid | + | defines a template where name is a complex expression | valid | + | infers a template with an expression | valid | + | refers to a hidden template | valid | + | refers to a local template | valid | + | refers to a missing template | invalid | + | refers to a template in a subdirectory | valid | + | refers to a template | valid | + | refers to a template with an expression | valid | + | refers to a template without an erb extension | valid | + | uses a missing inferred template | invalid | + | uses an inferred template | valid | + | uses a template from another cookbook | valid | + | includes a deploy resource that contains a template resource | valid | + | includes a template in the root of the templates directory | valid | diff --git a/spec/foodcritic/api_spec.rb b/spec/foodcritic/api_spec.rb index c8240035..b92fd11d 100644 --- a/spec/foodcritic/api_spec.rb +++ b/spec/foodcritic/api_spec.rb @@ -47,6 +47,7 @@ def parse_ast(str) :standard_cookbook_subdirs, :supported_platforms, :template_file, + :template_files, :template_paths, :templates_included, :valid_query?, From 70567718651f6e19d06d056a4738764c4fafe487 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 12 Sep 2016 16:17:14 -0700 Subject: [PATCH 4/4] Resolve chefstyle warnings Signed-off-by: Tim Smith --- lib/foodcritic/api.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/foodcritic/api.rb b/lib/foodcritic/api.rb index 416b6ad2..708aa7b8 100644 --- a/lib/foodcritic/api.rb +++ b/lib/foodcritic/api.rb @@ -361,11 +361,11 @@ def supported_platforms(ast) # Returns template source filenames if specified or implied by resource name def template_files(resource) - if resource['source'] - if resource['source'].respond_to?(:xpath) # source is an array - resource['source'].xpath('//array//tstring_content/@value').map {|x| x.to_s} + if resource["source"] + if resource["source"].respond_to?(:xpath) # source is an array + resource["source"].xpath("//array//tstring_content/@value").map { |x| x.to_s } else # source is a string - [resource['source']] + [resource["source"]] end elsif resource[:name] if resource[:name].respond_to?(:xpath)