cookbook created by chef generate cookbook fails foodcritic due to test directory

[mattstratton]

Description

After creating a skeleton cookbook with chef generate cookbook, it results in a cookbook that fails foodcritic validation due to some of the files in the test/ directory.

Example:

FC011: Missing README in markdown format: test/README.md:1
FC023: Prefer conditional attributes: ./test/recipes/default_test.rb:8
FC031: Cookbook without metadata file: test/metadata.rb:1
FC045: Metadata does not contain cookbook name: test/metadata.rb:1

ChefDK Version

ChefDK version:

Chef Development Kit Version: 0.17.17
chef-client version: 12.13.37
delivery version: master (f68e5c5804cd7d8a76c69b926fbb261e1070751b)
berks version: 4.3.5
kitchen version: 1.11.1

Platform Version

OS X Sierra

Replication Case

To replicate, create a new cookbook using chef generate cookbook and then execute foodcritic . in that directory.

[qubitrenegade]

We are experiencing this same issue on CentOS 6 with ChefDK 18.30.

I have to ask, why are we doing away with the test/integration/<test suite> model? For instance, I have a cookbook that installs a vendor package either via rpm or via tarball. In each case I have different integration tests that I want to run. It's not appropriate to test that we downloaded the tarball when installing the RPM version. Under ServerSpec this is easy, all I have to do is specify which suite I want to run. However, unless I'm misunderstanding, this seems to lump all InSpec integration tests into the same directory.

Another use case might be one cookbook that runs on different operating systems. If my cookbook runs on both Windows and Linux it's probably not appropriate to check for the existence of config files in /etc/blah on a Windows machine.

Wouldn't it make more sense and ease adoption if all the InSpec tests went in test/integration/default/inspec? This could allow InSpec and ServerSpec tests to live together while we transition from ServerSpec to InSpec.

Thanks! Jon A

[vinyar]

+1 This is causing delivery lint phase to explode What's happening is foodcritic thinks /test is a cookbook of its own, and it all goes sideways from there.

Synchronizing Cookbooks:
  - delivery-truck (2.2.2)
  - build_cookbook (0.1.0)
  - delivery-sugar (1.1.3)
Installing Cookbook Gems:
Compiling Cookbooks...
Converging 2 resources
Recipe: delivery-truck::lint
  * execute[lint_foodcritic_staging] action run

    ================================================================================
    Error executing action `run` on resource 'execute[lint_foodcritic_staging]'
    ================================================================================

    Mixlib::ShellOut::ShellCommandFailed
    ------------------------------------
    Expected process to exit with [0], but received '3'
    ---- Begin output of foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ ----
    STDOUT: FC011: Missing README in markdown format: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/README.md:1
    FC023: Prefer conditional attributes: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/recipes/default_test.rb:8
    FC031: Cookbook without metadata file: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/metadata.rb:1
    FC045: Metadata does not contain cookbook name: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/metadata.rb:1
    FC064: Ensure issues_url is set in metadata: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/metadata.rb:1
    FC065: Ensure source_url is set in metadata: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/metadata.rb:1
    STDERR: 
    ---- End output of foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ ----
    Ran foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ returned 3

    Resource Declaration:
    ---------------------
    # In /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/cache/cookbooks/delivery-truck/recipes/lint.rb

     20:   execute "lint_foodcritic_#{cookbook.name}" do
     21:     command "foodcritic #{foodcritic_fail_tags} #{foodcritic_tags} " \
     22:       "#{foodcritic_excludes} #{cookbook.path}"
     23:   end
     24: 

    Compiled Resource:
    ------------------
    # Declared in /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/cache/cookbooks/delivery-truck/recipes/lint.rb:20:in `block in from_file'

    execute("lint_foodcritic_staging") do
      action [:run]
      retries 0
      retry_delay 2
      default_guard_interpreter :execute
      command "foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/"
      backup 5
      returns 0
      declared_type :execute
      cookbook_name "delivery-truck"
      recipe_name "lint"
    end

    Platform:
    ---------
    x86_64-linux

Running handlers:
[2016-10-04T21:38:00+00:00] ERROR: Running exception handlers
Running handlers complete
[2016-10-04T21:38:00+00:00] ERROR: Exception handlers complete
Chef Client failed. 0 resources updated in 06 seconds
[2016-10-04T21:38:00+00:00] FATAL: Stacktrace dumped to /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/cache/chef-stacktrace.out
[2016-10-04T21:38:00+00:00] FATAL: Please provide the contents of the stacktrace.out file if you file a bug report
[2016-10-04T21:38:00+00:00] ERROR: execute[lint_foodcritic_staging] (delivery-truck::lint line 20) had an error: Mixlib::ShellOut::ShellCommandFailed: Expected process to exit with [0], but received '3'
---- Begin output of foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ ----
STDOUT: FC011: Missing README in markdown format: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/README.md:1
FC023: Prefer conditional attributes: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/recipes/default_test.rb:8
FC031: Cookbook without metadata file: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/metadata.rb:1
FC045: Metadata does not contain cookbook name: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/test/metadata.rb:1
FC064: Ensure issues_url is set in metadata: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/metadata.rb:1
FC065: Ensure source_url is set in metadata: /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/metadata.rb:1
STDERR: 
---- End output of foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ ----
Ran foodcritic -f correctness   /var/opt/delivery/workspace/ip-10-0-0-67.ec2.internal/alex_ent/twenty/staging/master/verify/lint/repo/ returned 3
[2016-10-04T21:38:01+00:00] FATAL: Chef::Exceptions::ChildConvergeError: Chef run process exited unsuccessfully (exit code 1)

[vinyar]

looks like this issue has been around for ever.. https://github.com/acrmp/foodcritic/issues/148

[vinyar]

@mattstratton did you see it as part of workflow?

I ran into the same thing, and the bandaid is to use use config.json. I am curious about having a conversation around what's the correct place to fix it - probably foodcritic as default behavior should not scan non-cookbook files

[mattstratton]

@vinyar nope. Just doing straight CLI foodcritic. I'm writing something up about chefdk and testing tools, without the context of Workflow, so config.json won't help.

[vinyar]

@mattstratton then just use -x to exclude a folder.

[tas50]

We need to fix this in Foodcritic itself. The default exclude needs to include the test dir. It causes issues in Delivery and also for any user just running Foodcritic from the CLI.

[charlesjohnson]

chef -v
Chef Development Kit Version: 1.1.16
chef-client version: 12.17.44
delivery version: master (83358fb62c0f711c70ad5a81030a6cae4017f103)
berks version: 5.2.0
kitchen version: 1.14.2

GOOD NEWS: Foodcritic no longer fails due to the test directory.

BAD NEWS: Foodcritic now fails for a completely different reason, that is arguably Foodcritic's fault, not the cookbook's fault:

➜  chef generate cookbook baz
Generating cookbook baz
- Ensuring correct cookbook file content
- Committing cookbook files to git
- Ensuring delivery configuration
- Ensuring correct delivery build cookbook content
- Adding delivery configuration to feature branch
- Adding build cookbook to feature branch
- Merging delivery content feature branch to master

Your cookbook is ready. Type `cd baz` to enter it.

*snip*

➜  foodcritic -f correctness baz
FC064: Ensure issues_url is set in metadata: baz/metadata.rb:1
FC065: Ensure source_url is set in metadata: baz/metadata.rb:1

[charlesjohnson]

Should we keep looking at this here, or should we take it to https://github.com/acrmp/foodcritic/issues/503 ?

[qubitrenegade]

➜ foodcritic -f correctness baz FC064: Ensure issues_url is set in metadata: baz/metadata.rb:1 FC065: Ensure source_url is set in metadata: baz/metadata.rb:1

This to me is a perfectly reasonable reason for FoodCritic to fail. It would be nice if you could set that somehow (CLI flag and/or knife.rb config), however, I think this is a markedly different failure than failing because the fundamental layout of your cookbook dir structure is "incorrect".

[nathenharvey]

Neither FC064 nor FC065 are tagged with correctness.

Output from foodcritic is not an indication of failure, the exit code should be checked for that.

If you want to ensure both the output and the command are only showing results of testing for rules tagged with correctness you need to use foodcritic -f correctness -t correctness baz

[nathenharvey]

I believe this issue can now be closed. What do you say @mattstratton?

[cheeseplus]

Foodcritic now excludes test/ dirs by default as of https://github.com/acrmp/foodcritic/commit/aa69442ea125b083bc7178d277f786a63601d80b

[tas50]

Expect a release of Foodcritic within the next few days, which will ship in next months Chef-DK

[vinyar]

Still an issue in ChefDK 1.3.43

$ chef generate cookbook trash                                                                     [ruby-2.3.1p112]
Generating cookbook trash
...
Your cookbook is ready. To setup the pipeline, type `cd trash`, then run `delivery init`
alexvinyar@seavinyar02 ~/Documents/git/cookbooks $ cd trash                                                                                         [ruby-2.3.1p112]
alexvinyar@seavinyar02 ~/Documents/git/cookbooks/trash (master)$ foodcritic . -f correctness                                                        [ruby-2.3.1p112]
Checking 2 files
x.
FC064: Ensure issues_url is set in metadata: ./metadata.rb:1
FC065: Ensure source_url is set in metadata: ./metadata.rb:1
FC067: Ensure at least one platform supported in metadata: ./metadata.rb:1
alexvinyar@seavinyar02 ~/Documents/git/cookbooks/trash (master)$ chef -v                                                                            [ruby-2.3.1p112]
Chef Development Kit Version: 1.3.43
chef-client version: 12.19.36
delivery version: master (dd319aa632c2f550c92a2172b9d1226478fea997)
berks version: 5.6.4
kitchen version: 1.16.0
alexvinyar@seavinyar02 ~/Documents/git/cookbooks/trash (master)$ chef gem list foodc                                                                [ruby-2.3.1p112]

*** LOCAL GEMS ***

foodcritic (10.2.2)

The interesting thing, is that with foodcritic 11.0.0 it actually gets a tiny bit worse as it's now failing on FC078:

$ foodcritic -V              
foodcritic 11.0.0

foodcritic . -f correctness                                                        [ruby-2.3.1p112]
Checking 2 files
x.
FC008: Generated cookbook metadata needs updating: ./metadata.rb:2
FC008: Generated cookbook metadata needs updating: ./metadata.rb:3
FC064: Ensure issues_url is set in metadata: ./metadata.rb:1
FC065: Ensure source_url is set in metadata: ./metadata.rb:1
FC067: Ensure at least one platform supported in metadata: ./metadata.rb:1
FC078: Ensure cookbook shared under an OSI-approved open source license: ./metadata.rb:1

[charlesjohnson]

At this point our own tooling is executing foodcritic with foodcritic . -f any -t "~supermarket".

Is there a strong need from customers to pass out of the box without setting the metadata correctly for Supermarket?

[tas50]

So we're going to have a hard time with this going forward. Foodcritic sat idle for about 2 years, but I've added 7 rules in the last week. There's going to be a lot more rules coming, which will constantly add new requirements. Also keep in mind that and and out of the box cookbook SHOULD NOT pass. It's not a functional cookbook. It's just a generated skeleton that needs modification before it's ready.

[charlesjohnson]

We might need to find a compromise by which out-of-the-box cookbooks can pass foodcritic in an Automate Workflow context.

Because Automate uses the initial project skeleton commit to test out the pipeline, if there's a pipeline failure in the foodcritic phase, then that'll break the pipeline until the project passes.

Not a deal breaker, but I think it impacts the use cases that @vinyar sees.

@vinyar: Am I putting words in your mouth?

[qubitrenegade]

@charlesjohnson when this issue was first opened the issue was that foodcritic was generating erroneous errors based on changes to the default test framework (i.e.: the change over from ServerSpec to InSpec). The change in directory structure caused FoodCritic to incorrectly find that there was no README.md, etc. for the "cookbook" in the tests/ directory.

The errors posted by @vinyar are not erroneous however. These are errors that I would expect to receive until you fix them. These indicate there is something wrong with your cookbook. Your cookbook should have an issues and source URL, even if they're the same (they are in our environment for instance), your cookbook should declare platforms it supports, etc. As these errors are not erroneous and are reporting things that are wrong in the cookbook, I don't think these failures really fit in with the original bug report.

In my opinion, the fix for the original issue reported by @mattstratton an update to ChefDK, the fix for @vinyar problem is to update his metadata.

I would even argue further that these SHOULD cause automate/foodcritic to explode, if I'm collaborating with a team (pushing cookbooks through Automate or not) then it's even more important to tell people where you can find source, what platforms my cookbook supports, etc. Our standard for communicating that information is to put it in the Metadata.

(Having said that, it would be nice to be able to set these as part of the chef generate cookbook process either with a CLI flag or by setting attributes in your .chef/knife.rb or .chef/config.rb. but that's perhaps a topic for another thread)

[charlesjohnson]

@mattstratton is your issue resolved?

@vinyar What do you think of the discussion since your update?

[mattstratton]

From my perspective, my issue has been resolved.

[cheeseplus]

Initial issue is closed and the other issue is both by design and unrelated.

[vinyar]

@charlesjohnson the use-case you described is what I was after. In the grand scheme of things, I could argue that my version of the bug mostly affects newer customers as it takes more gymnastics to get going, resulting in a very rocky early experience. Advanced users will all run custom cookbook generators anyway.

If the initial commit fails, the config.json will need to be modified. That puts onus on the user to re-enable testing with the very next commit, or in a less desireable case - at some point in the future.