Skip to content

Commit 0df0658

Browse files
sdssds
committed
Enhance Foodcritic hook with better support for both modes
Since we use a monolithic Chef repo at Brigade, it would be really nice to be able to use this hook as a built-in. The original implementation came close, but there were a few rough edges. Add some better tests and additional documentation so it's clear how to run the hook in standard "one cookbook" mode, or when working with multiple cookbooks in a single repo. While here I enabled the `expect` mock syntax for RSpec. I'll try to start using that going forward.
1 parent 10a2242 commit 0df0658

File tree

4 files changed

+200
-93
lines changed

4 files changed

+200
-93
lines changed

‎config/default.yml

+1-2
Original file line numberDiff line numberDiff line change
@@ -258,9 +258,8 @@ PreCommit:
258258
enabled: false
259259
description: 'Analyze with Foodcritic'
260260
required_executable: 'foodcritic'
261-
flags: ['-f', 'any']
261+
flags: ['--epic-fail=any']
262262
install_command: 'gem install foodcritic'
263-
include: '**/*.rb'
264263

265264
ForbiddenBranches:
266265
enabled: false

‎lib/overcommit/hook/pre_commit/foodcritic.rb

+99-47
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,106 @@
11
module Overcommit::Hook::PreCommit
2-
# Runs `foodcritic` against any modified Ruby files from chef directory structure.
2+
# Runs `foodcritic` against any modified Ruby files from Chef directory structure.
33
#
44
# @see http://www.foodcritic.io/
55
#
6-
# Provides three configuration options:
6+
# There are two "modes" you can run this hook in based on the repo:
77
#
8-
# * `cookbooks_directory`
9-
# If not set, or set to false, the current directory will be treated as a cookbook directory
10-
# (the one containing recipes, libraries, etc.)
11-
# If set to path (absolute or relative), hook will interpret it as a cookbooks directory
12-
# (so all each subdirectory will be treated as separate cookbook)
13-
# * `environments_directory`
14-
# If provided, the given path will be treated as environments directory
15-
# * `roles_directory`
16-
# If provided, the given path will be treated as roles directory
8+
# SINGLE COOKBOOK REPO MODE
9+
# -------------------------
10+
# The default. Use this if your repository contains just a single cookbook,
11+
# i.e. the top-level repo directory contains directories called `attributes`,
12+
# `libraries`, `recipes`, etc.
1713
#
18-
# By default, none of those options is set, which means, the repo directory will be treaded
19-
# as a cookbook directory (with recipes, libraries etc.)
14+
# To get this to work well, you'll want to set your Overcommit configuration
15+
# for this hook to something like:
2016
#
21-
# Example:
17+
# PreCommit:
18+
# Foodcritic:
19+
# enabled: true
20+
# include:
21+
# - 'attributes/**/*'
22+
# - 'definitions/**/*'
23+
# - 'files/**/*'
24+
# - 'libraries/**/*'
25+
# - 'providers/**/*'
26+
# - 'recipes/**/*'
27+
# - 'resources/**/*'
28+
# - 'templates/**/*'
2229
#
23-
# Foodcritic:
24-
# enabled: true
25-
# cookbooks_directory: './cookbooks'
26-
# environments_directory: './environments'
27-
# roles_directory: './roles'
30+
# MONOLITHIC REPO MODE
31+
# --------------------
32+
# Use this if you store multiple cookbooks, environments, and roles (or any
33+
# combination thereof) in a single repository.
34+
#
35+
# There are three configuration options relevant here:
36+
#
37+
# * `cookbooks_directory`
38+
# When set, hook will treat the path as a directory containing cookbooks.
39+
# Each subdirectory of this directory will be treated as a separate
40+
# cookbook.
41+
#
42+
# * `environments_directory`
43+
# When set, hook will treat the path as a directory containing environment
44+
# files.
45+
#
46+
# * `roles_directory`
47+
# When set, hook will treat the given path as a directory containing role
48+
# files.
49+
#
50+
# In order to run in monolithic repo mode, YOU MUST SET `cookbooks_directory`.
51+
# The other configuration options are optional, if you happen to store
52+
# environments/roles in another repo.
53+
#
54+
# To get this to work well, you'll want to set your Overcommit configuration
55+
# for this hook to something like:
56+
#
57+
# PreCommit:
58+
# Foodcritic:
59+
# enabled: true
60+
# cookbooks_directory: 'cookbooks'
61+
# environments_directory: 'environments'
62+
# roles_directory: 'roles'
63+
# include:
64+
# - 'cookbooks/**/*'
65+
# - 'environments/**/*'
66+
# - 'roles/**/*'
67+
#
68+
# ADDITIONAL CONFIGURATION
69+
# ------------------------
70+
# You can disable rules using the `flags` hook option. For example:
71+
#
72+
# PreCommit:
73+
# Foodcritic:
74+
# enabled: true
75+
# ...
76+
# flags:
77+
# - '--epic-fail=any'
78+
# - '-t~FC011' # Missing README in markdown format
79+
# - '-t~FC064' # Ensure issues_url is set in metadata
80+
#
81+
# Any other command line flag supported by the `foodcritic` executable can be
82+
# specified here.
83+
#
84+
# If you want the hook run to fail (and not just warn), set the `on_warn`
85+
# option for the hook to `fail`:
86+
#
87+
# PreCommit:
88+
# Foodcritic:
89+
# enabled: true
90+
# on_warn: fail
91+
# ...
92+
#
93+
# This will treat any warnings as failures and cause the hook to exit
94+
# unsuccessfully.
2895
class Foodcritic < Base
2996
def run
30-
args = modified_environments_args + modified_roles_args + modified_cookbooks_args
31-
32-
args += applicable_files.reject do |file|
33-
%w[spec test].any? { |dir| file.include?("#{File::SEPARATOR}#{dir}#{File::SEPARATOR}") }
34-
end - modified_environments - modified_roles if modified_cookbooks.empty?
35-
97+
args = modified_cookbooks_args + modified_environments_args + modified_roles_args
3698
result = execute(command, args: args)
3799

38100
if result.success?
39101
:pass
40102
else
41-
return [:warn, result.stdout]
103+
return [:warn, result.stderr + result.stdout]
42104
end
43105
end
44106

@@ -50,31 +112,25 @@ def directories_changed(dir_prefix)
50112
map { |path| path.gsub(%r{^#{dir_prefix}/}, '') }.
51113
group_by { |path| path.split('/').first }.
52114
keys.
53-
map { |cookbook| File.join(dir_prefix, cookbook) }
115+
map { |path| File.join(dir_prefix, path) }
54116
end
55117

56118
def modified_environments_args
57-
modified_environments.map { |env| %W[-E #{env}] }.flatten
119+
modified('environments').map { |env| %W[-E #{env}] }.flatten
58120
end
59121

60122
def modified_roles_args
61-
modified_roles.map { |role| %W[-R #{role}] }.flatten
123+
modified('roles').map { |role| %W[-R #{role}] }.flatten
62124
end
63125

64126
def modified_cookbooks_args
65-
modified_cookbooks.map { |cookbook| %W[-B #{cookbook}] }.flatten
66-
end
67-
68-
def modified_environments
69-
modified 'environments'
70-
end
71-
72-
def modified_roles
73-
modified 'roles'
74-
end
75-
76-
def modified_cookbooks
77-
modified 'cookbooks'
127+
# Return the repo root if repository contains a single cookbook
128+
if !config['cookbooks_directory'] || config['cookbooks_directory'].empty?
129+
['-B', Overcommit::Utils.repo_root]
130+
else
131+
# Otherwise return all modified cookbooks in the cookbook directory
132+
modified('cookbooks').map { |cookbook| ['-B', cookbook] }.flatten
133+
end
78134
end
79135

80136
def modified(type)
@@ -85,11 +141,7 @@ def modified(type)
85141

86142
def full_directory_path(config_option)
87143
return config[config_option] if config[config_option].start_with?(File::SEPARATOR)
88-
File.absolute_path(File.join(repo_root, config[config_option]))
89-
end
90-
91-
def repo_root
92-
Overcommit::Utils.repo_root
144+
File.absolute_path(File.join(Overcommit::Utils.repo_root, config[config_option]))
93145
end
94146
end
95147
end

‎spec/overcommit/hook/pre_commit/foodcritic_spec.rb

+99-43
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,16 @@
11
require 'spec_helper'
22

33
describe Overcommit::Hook::PreCommit::Foodcritic do
4-
let(:config) do
5-
Overcommit::ConfigurationLoader.default_configuration.merge(
6-
Overcommit::Configuration.new('PreCommit' => {
7-
'Foodcritic' => {
8-
'cookbooks_directory' => '.',
9-
'environments_directory' => '.',
10-
'roles_directory' => '.'
11-
}
12-
})
13-
)
14-
end
154
let(:context) { double('context') }
16-
let(:applicable_files) { %w[file1.rb file2.rb] }
5+
let(:result) { double(success?: true) }
176
subject { described_class.new(config, context) }
187

198
before do
20-
subject.stub(:applicable_files).and_return(applicable_files)
21-
subject.stub(:modified).and_return(applicable_files)
9+
modified_files = applicable_files.map do |file|
10+
File.join(Overcommit::Utils.repo_root, file)
11+
end
12+
subject.stub(:applicable_files).and_return(modified_files)
13+
allow(subject).to receive(:execute).and_return(result)
2214
end
2315

2416
around do |example|
@@ -27,43 +19,107 @@
2719
end
2820
end
2921

30-
before do
31-
args = %w[-E -R -B].map { |arg| applicable_files.map { |file| [arg, file] } }.flatten
32-
subject.stub(:execute).with(%w[foodcritic -f any], args: args).and_return(result)
33-
end
22+
context 'when working in a single cookbook repository' do
23+
let(:config) { Overcommit::ConfigurationLoader.default_configuration }
3424

35-
context 'and has 2 suggestions for metadata improvement' do
36-
let(:result) do
37-
double(
38-
success?: false,
39-
stdout: <<-EOF
40-
FC008: Generated cookbook metadata needs updating: file1.rb:24
41-
FC029: No leading cookbook name in recipe metadata: file2.rb:37
42-
EOF
43-
)
44-
end
25+
context 'and files have changed' do
26+
let(:applicable_files) do
27+
[
28+
'metadata.rb',
29+
File.join('recipes', 'default.rb'),
30+
]
31+
end
32+
33+
it 'passes the repository root as the cookbook path' do
34+
expect(subject).to receive(:execute).
35+
with(subject.command,
36+
hash_including(args: ['-B', Overcommit::Utils.repo_root])).
37+
and_call_original
38+
subject.run
39+
end
40+
41+
context 'and Foodcritic returns an unsuccessful exit status' do
42+
let(:result) do
43+
double(
44+
success?: false,
45+
stderr: '',
46+
stdout: <<-EOF,
47+
FC023: Prefer conditional attributes: recipes/default.rb:11
48+
FC065: Ensure source_url is set in metadata: metadata.rb:1
49+
EOF
50+
)
51+
end
4552

46-
it { should warn }
53+
it { should warn }
54+
end
55+
56+
context 'and Foodcritic returns a successful exit status' do
57+
it { should pass }
58+
end
59+
end
4760
end
4861

49-
context 'and has single suggestion for template' do
50-
let(:result) do
51-
double(
52-
success?: false,
53-
stdout: <<-EOF
54-
FC034: Unused template variables: file1.rb:64
55-
EOF
62+
context 'when working in a repository with many cookbooks' do
63+
let(:config) do
64+
Overcommit::ConfigurationLoader.default_configuration.merge(
65+
Overcommit::Configuration.new(
66+
'PreCommit' => {
67+
'Foodcritic' => {
68+
'cookbooks_directory' => 'cookbooks',
69+
'environments_directory' => 'environments',
70+
'roles_directory' => 'roles',
71+
}
72+
}
73+
)
5674
)
5775
end
5876

59-
it { should warn }
60-
end
77+
context 'and multiple cookbooks, environments, and roles have changed' do
78+
let(:applicable_files) do
79+
[
80+
File.join('cookbooks', 'cookbook_a', 'metadata.rb'),
81+
File.join('cookbooks', 'cookbook_b', 'metadata.rb'),
82+
File.join('environments', 'production.json'),
83+
File.join('environments', 'staging.json'),
84+
File.join('roles', 'role_a.json'),
85+
File.join('roles', 'role_b.json'),
86+
]
87+
end
6188

62-
context 'and does not have any suggestion' do
63-
let(:result) do
64-
double(success?: true, stdout: "\n")
65-
end
89+
it 'passes the modified cookbook, environment, and role paths' do
90+
expect(subject).to receive(:execute).
91+
with(subject.command,
92+
hash_including(args: [
93+
'-B', File.join(Overcommit::Utils.repo_root, 'cookbooks', 'cookbook_a'),
94+
'-B', File.join(Overcommit::Utils.repo_root, 'cookbooks', 'cookbook_b'),
95+
'-E', File.join(Overcommit::Utils.repo_root, 'environments', 'production.json'),
96+
'-E', File.join(Overcommit::Utils.repo_root, 'environments', 'staging.json'),
97+
'-R', File.join(Overcommit::Utils.repo_root, 'roles', 'role_a.json'),
98+
'-R', File.join(Overcommit::Utils.repo_root, 'roles', 'role_b.json'),
99+
])).and_call_original
100+
subject.run
101+
end
66102

67-
it { should pass }
103+
context 'and Foodcritic returns an unsuccessful exit status' do
104+
let(:result) do
105+
double(
106+
success?: false,
107+
stderr: '',
108+
stdout: <<-EOF,
109+
FC023: Prefer conditional attributes: cookbooks/cookbook_a/recipes/default.rb:11
110+
FC065: Ensure source_url is set in metadata: cookbooks/cookbook_b/metadata.rb:1
111+
EOF
112+
)
113+
end
114+
115+
it { should warn }
116+
end
117+
118+
context 'and Foodcritic returns a successful exit status' do
119+
let(:result) { double(success?: true) }
120+
121+
it { should pass }
122+
end
123+
end
68124
end
69125
end

‎spec/spec_helper.rb

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
end
3939

4040
config.mock_with :rspec do |c|
41-
c.syntax = :should
41+
c.syntax = [:expect, :should]
4242
end
4343

4444
config.around(:each) do |example|

0 commit comments

Comments
 (0)