Skip to content
This repository

Linter should check just cookbook directories #37

Open
juanje opened this Issue · 18 comments

5 participants

Juanje Ojeda Andrew Crump Chris Griego Fletcher Nichol Jamie Winsor
Juanje Ojeda
juanje commented

Hi Andrew,

I've seen this before, but some friends where trying Foodcritic in addition of tests and they have troubles because Foodcritic treat the non known directories as cookbooks.

I mean, Foodcritic treat every cookbook_path you pass to him as a possible cookbook, but also as a directory with cookbooks. As result of that, when one cookbook have any directory different of attributes, recipes, resourses, providers, files and templates, it try to check the directory as a cookbook.

With testing it could be a problem because we try to use cookbbook/tests or cookbook/spec and put inside the tree of tests. But when Foodcritic see the directory try to check cookbook/tests and raises false errors.

I didn't do a pull-request because I didn't know what you think about or what is the approach you like the most. I tried to check if the cokkbook_path has a metadata.rb (which every cookbook should has) to check if is a cookbook or a root directory for more cookbooks:

diff --git a/lib/foodcritic/linter.rb b/lib/foodcritic/linter.rb
index 0fda9c1..a9f78fd 100644
--- a/lib/foodcritic/linter.rb
+++ b/lib/foodcritic/linter.rb
@@ -155,8 +155,11 @@ module FoodCritic
       dirs.each do |dir|
         if File.directory? dir
           cookbook_glob = '{attributes,providers,recipes,resources}/*.rb'
-          files += Dir.glob(File.join(dir, cookbook_glob)) +
-            Dir.glob(File.join(dir, "*/#{cookbook_glob}"))
+          if File.exists? File.join(dir, 'metadata.rb')
+            files += Dir.glob(File.join(dir, cookbook_glob))
+          else
+            files += Dir.glob(File.join(dir, "*/#{cookbook_glob}"))
+          end
         else
           files << dir
         end

This works for me, but I don't know if you like the approach. Also I wasn't sure the best place to put a test for that.

Sorry for my awful explanation but I hope you'll get my point.

Cheers

Andrew Crump
Owner
acrmp commented

Hi Juanje,

This change looks good to me! If you can submit a pull request for this that would be really awesome. For testing it would probably work best as a new cucumber feature. I can see that we might want to test:

  • Cookbook with no additional sub directories
  • Cookbook with additional sub directories
  • Cookbook with a nested sub directory that is itself a cookbook
  • Cookbook without a metadata.rb

@fnichol I noticed that you are preparing a sandbox for foodcritic in your Rakefile. Is this related?
https://github.com/fnichol/chef-rvm/blob/f7884a05287d1e5caa15d947bd0f3899c6a7ead5/Rakefile#L21-29

We should probably get a Rake tasklib added to foodcritic too to make it as easy as possible to use.

Cheers,

Andrew.

Juanje Ojeda
juanje commented

Great! :-)

I'll create those tests and I'll submit a pull request with the tests and code.

Cheers

Juanje Ojeda
juanje commented

@acrmp, I'm working on it, but it seems like the code I proposed breaks the las feature you wrote. The features/multiple_paths.feature one:

Feature: Multiple paths

  In order to avoid needing to run foodcritic multiple times
  As a developer
  I want to lint multiple paths at once

  Scenario: Linting multiple individual cookbooks                                         # features/multiple_paths.feature:7
    Given a cookbook with a single recipe that reads node attributes via symbols only     # features/step_definitions/cookbook_steps.rb:577
    And another cookbook with a single recipe that reads node attributes via strings only # features/step_definitions/cookbook_steps.rb:577
    When I check both cookbooks                                                           # features/step_definitions/cookbook_steps.rb:793
    Then the node access warning 001 should be displayed                                  # features/step_definitions/cookbook_steps.rb:906
      expected "" to include "FC001: Use strings in preference to symbols to access node attributes: cookbooks/example/recipes/default.rb:1\n" (RSpec::Expectations::ExpectationNotMetError)
      ./features/support/command_helpers.rb:141:in `expect_output'
      ./features/support/command_helpers.rb:89:in `expect_warning'
      ./features/step_definitions/cookbook_steps.rb:919:in `/^the (?:[a-zA-Z \-_]+) warning ([0-9]+) should (not )?be displayed(?: against the (attributes|definition|metadata|provider|resource|README.md|README.rdoc) file)?( below)?$/'
      features/multiple_paths.feature:11:in `Then the node access warning 001 should be displayed'

Failing Scenarios:
cucumber features/multiple_paths.feature:7 # Scenario: Linting multiple individual cookbooks

1 scenario (1 failed)
4 steps (1 failed, 3 passed)
0m0.021s

I'll look into that later, but I'd appreciate if you could point me to the right direction.

Thanks

Andrew Crump
Owner
acrmp commented

It looks like the change will cause a lot of the existing tests to fail because they don't write out a metadata.rb when creating example cookbooks.

You'll need to modify the test helpers to create this file to get the tests passing. The test helpers live here:
https://github.com/acrmp/foodcritic/blob/master/features/support/cookbook_helpers.rb

Thanks,

Andrew.

Juanje Ojeda
juanje commented

Right! Of course, I was blind...

I will added it with the rest of the changes.

Thanks!

Juanje Ojeda
juanje commented

Hi Andrew,

I'm sorry, I've been a bit off last week... :-/

I'm back with the bug. I tried to do something generic at the cookbook_helpers.rb but there are a lot of step_definitions that use write_recipe, write_file or even write_metadata. I added the write_metadata to the methods that create a cookboks, but I had also to add it to a bunch of steps.

Here you can see those changes. I got rid of the errors from cucumber with this.

There is also one Scenario that need the metadata.rb not to be written:

  Scenario: Cookbook has no metadata file
    Given a cookbook that does not have defined metadata
    When I check the cookbook
    Then the undeclared dependency warning 007 should not be displayed
     And no error should have occurred

but I think this is wrong. If the metadata.rb doesn't exist, the dependency is not declared... Anyway, It should probably show a warning about the metadata itself.

This reminds me... AFAIK, there is no rule or warning for no having metadata.rb file. Don't you think it could be a good idea to have one?
For example, with the change I proposed a cookbook without metadata.rb file won't be linted, but it won't say anything either. This could be a side effect you don't want to have.

As long as we have now multiple paths I think we could avoid to try subtrees of cookbboks. Right now you can do:

$ foodcritic cookbooks/*

istead of:

$ foodcritic cookbooks/

and it will lint all the cookbooks under cookbooks. If we check for the metadata.rb inside the individual cookbooks and some of them doesn't have it, they won't be linted.

IMHO, we should just remove the subtree check and let the user add the cookbooks as s/he likes. It would let the users to test cookbooks without metadata.rb if they really want to do that.

What do you think about? Then we can avoid all the write_metadata mess I linked to you before.

I've just tried the small change:

diff --git a/lib/foodcritic/linter.rb b/lib/foodcritic/linter.rb
index 0fda9c1..83cf6ef 100644
--- a/lib/foodcritic/linter.rb
+++ b/lib/foodcritic/linter.rb
@@ -155,8 +155,7 @@ module FoodCritic
       dirs.each do |dir|
         if File.directory? dir
           cookbook_glob = '{attributes,providers,recipes,resources}/*.rb'
-          files += Dir.glob(File.join(dir, cookbook_glob)) +
-            Dir.glob(File.join(dir, "*/#{cookbook_glob}"))
+          files += Dir.glob(File.join(dir, cookbook_glob))
         else
           files << dir
         end

and it works as I expected.

$ foodcritic cookbooks/my_cookbook

checks my_cookbook, which is a cookbook.

$ foodcritic cookbooks/

doesn't check anything, because that is the parent directoy to the actual cookbooks.

$ foodcritic cookbooks/*

does lint all the cookbooks inside of the cookbooks/ directory.

Sorry for the long comment, I trying to see the different posibilities and side effects for the best of Foodcritic, which I like so much.

Cheers and thanks for your patience :-)

Andrew Crump
Owner
acrmp commented

Hi Juanje,

Hope you're ok. Thanks for your efforts so far.

but I think this is wrong. If the metadata.rb doesn't exist, the dependency is not declared...

I think you're right and this is backwards - good spotting.

This reminds me... AFAIK, there is no rule or warning for no having metadata.rb file. Don't you
think it could be a good idea to have one?

Definitely!

IMHO, we should just remove the subtree check and let the user add the cookbooks as s/he likes.
It would let the users to test cookbooks without metadata.rb if they really want to do that.
What do you think about? Then we can avoid all the write_metadata mess I linked to you before.

This would be a pretty big behaviour change for users that are expecting foodcritic to lint all the cookbooks under a cookbook_path. I think I prefer the convenience of allowing only the top-level directory to be passed in. If we were to do this we'd need to bump the major version number to signal this at minimum.

Sorry for the long comment, I trying to see the different posibilities and side effects for the
best of Foodcritic, which I like so much.

Thanks for all the ideas. You rock.

Cheers,

Andrew.

Juanje Ojeda
juanje commented
Juanje Ojeda
juanje commented

Never mind, I've just created a pull-request (#43) for checking the metadata.rb file :-)

Once that rule be integrated, I'll finish this part, with will be easier to address and test.

Cheers

Chris Griego

+1 for this. We use Bundler in our CI environment with cookbook linting and discovered, thanks to the new FC030 breakpoint rule in 1.4.0, that foodcritic was parsing ASTs for every Ruby file in the vendor/bundle directory. The build_xml method hit an infinite loop inside the Gherkin lexer. :) We've downgraded to a version without FC030 as a workaround to get the build working again.

Andrew Crump
Owner
acrmp commented

Wow, that is nasty. Thanks for reporting this Chris.

As an alternative to downgrading in the interim you can specifically exclude that rule with:

foodcritic -t ~FC030 my_cookbook

The globbing that is causing the vendor directory to be parsed is in the rule itself, so even if the linter excludes the paths the exclusions are ignored. I made some changes in 094ec2a that allow you to specify exclusions when using foodcritic from rake that this rule would also defeat.

That gives me:

  • Investigate build_xml stack overflow.
  • Add an integration test for a vendored bundle with a Cucumber dependency.
  • Remove the globbing from FC019 and FC030 and add library and metadata to the DSL.
  • Update existing metadata rules to use metadata.

Any comments?

Cheers,

Andrew.

Andrew Crump acrmp referenced this issue from a commit
Andrew Crump Only lint cookbook directories, refs #37.
- Extend DSL for library and metadata, removing problematic globbing.
- Readability improvements.
- Add Rocco for documentation and rework comments.
- Move rake test helpers into their own module.
efd3d9c
Juanje Ojeda
juanje commented

Hi Andrew,

I saw your commits and you did pretty good job :-)

Sorry I didn't say nothing in a while. I tried different approach but I was finding more and more ramifications. I got a bit stuck. Finally I got this weekend working with some tests, but your changes are more better :-P

I think your changes fixed the main part of the problem and improved a lot the solution, but I still get some issues.

I saw you added :exclude_paths to the options fot the rake tasks, but not for the command line, so you can't exclude specific paths when you are calling foodcritic from the command line.
I tried to add the option and is working as I was expected, but I don't know if you did this on purpose or what.

Another thing I detected (and this was one thing I was fighting with for awhile las week...) is that file_to_process() detect better the cookbooks dirs, but still there are some cases where it fails.
For example, if you have a directory tree like this:


/tmp/example/a
├── metadata.rb
├── providers
│   └── default.rb
└── test
    └── recipes
        └── default.rb

when you run foodcritic, you'll get this:


jojeda@dell:~/git/foodcritic$ bundle exec bin/foodcritic /tmp/example/a 
FC001: Use strings in preference to symbols to access node attributes: /tmp/example/a/test/recipes/default.rb:1
FC011: Missing README in markdown format: /tmp/example/a/README.md:1
FC011: Missing README in markdown format: /tmp/example/a/test/README.md:1
FC017: LWRP does not notify when updated: /tmp/example/a/providers/default.rb:1
FC031: Cookbook without metadata file: /tmp/example/a/test/metadata.rb:1

It shouldn't try to lint a cookbook inside another cookbook. Here it lint /tmp/example/a (which is actually a cookbook) and /tmp/example/a/test/ (which isn't and is inside one cookbook).

I was creating test (features) for this and testing my changes, so I'll try to use them with your new changes so we have this working properly, if you agree.

I was thinking of this cases as a simples ones:

  • A single directory which is a cookbook
  • A single directory which isn't a cookbook
  • A single directory which contains a single subdirectory which is a cookbook
  • A single directory which contains a single subdirectory which isn't a cookbook
  • A single directory which is a cookbook and contains a subdirectory which looks like a cookbook

What do you think?

By the way, thanks for all the good work you're doing :-)

Cheers

Andrew Crump
Owner
acrmp commented

Hi Juanje,

Thanks for the feedback. A sanity check is very welcome - particularly now as my wife has given birth to our first child recently so sleep and focus are both a little elusive. :baby:

There was no reason for not including exclude_paths as a command line option. I hadn't got around to it yet and a pull request for that would be brilliant.

The rationale for the exclude feature is to exclude paths that appear to be cookbooks but are in fact just similarly named. I want to avoid the need for the sandbox copy step in the Rake example that people are using.

An alternative situation is that the nested paths are in fact a valid cookbook that we do want to lint - the example here is of a 'setup cookbook' that ships with the enclosing cookbook to do any extra setup required for integration tests.

If you have any feedback on the Rake task that would be great too - I'm concerned that the options hash may be confusing:
https://github.com/acrmp/foodcritic/blob/efd3d9cf24f0d6f63d4bdd3c03eb2675ba70c64a/features/build_framework_support.feature#L67

Thanks,

Andrew.

Fletcher Nichol

@acrmp Yeah that sandbox step is on me… I've been meaning to circle back on that but I've been following this issue thread instead. :smile: Should I be looking in/testing against rake_task.rb for that Rake task?

Andrew Crump
Owner
acrmp commented

@fnichol If you can find some time to do some testing of your projects with the rake task that would be sweet. There are the start of docs for it here.
http://acrmp.github.com/foodcritic/#build-framework-integration

@reset shipped Thor support pretty much in parallel so you might want to try that out too. It would be nice if the interfaces were similar.

Juanje Ojeda
juanje commented

Andrew, congrats for the baby! :smile:

I'll happily send you a pull-request for the :exclude_paths, which is a options I was always missing, by the way.

So, do you think the nested cookbook is a possible valid case and is better allowed it? I guess that the :exclude_paths could be enough for the other cases.
Do I leave it like that then?

@fnichol, nice to see you in this thread :-)

Jamie Winsor
reset commented
Andrew Crump
Owner
acrmp commented

@reset That's awesome. Thanks for sharing Thor support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.