Chef Review Guidelines.md

Chef Cookbook Review Guide

Things to look for when reviewing cookbook pull requests (examples are linked where explanation is necessary):

1. Attributes should be preferred over literals in resource properties

2. ChefSpec unit tests are included for any logic branches in recipes

3. Any plain Ruby is defined as a library method with rspec unit tests

4. Config files should be rendered by iterating over node attributes (or better, with a library) rather than variables and a template

Examples

Attributes over literals

Bad

# recipe.rb
file '/etc/file' do
  content 'File contents'
  mode '0755'
  owner 'admin'
  group 'admin'
end

Good

# attributes.rb
default['etc_file']['content'] = 'File contents'
default['etc_file']['mode'] = '0755'
default['etc_file']['owner'] = 'admin'
default['etc_file']['group'] = 'admin'

# recipe.rb
file '/etc/file' do
  content node['etc_file']['content']
  mode node['etc_file']['mode']
  owner node['etc_file']['owner']
  group node['etc_file']['group']
end

Ruby in libraries

Bad

# some_recipe.rb
Dir.glob('/etc/*').select { |f| ::File.file?(f) }.each do |etc_file|
  file etc_file do
    owner 'root'
    group 'root'
  end
end

Good

# libraries/some_cookbook_helper.rb
module SomeCookbookHelper
  module_function
  def etc_files
    Dir.glob('/etc/*').select { |f| ::File.file?(f) }
  end
end

# recipes/some_recipe.rb
SomeCookbookHelper.etc_files.each do |etc_file|
  file etc_file do
    owner 'root'
    group 'root'
  end
end

# spec/libraries/some_cookbook_helper_spec.rb
describe(SomeCookbookHelper) do
  before do
    allow(Dir).to receive(:glob).with('/etc/*').and_return(
      %w[/etc/file /etc/directory]
    )
    allow(::File).to receive(:file?).with('/etc/file').and_return(true)
    allow(::File).to receive(:file?).with('/etc/directory').and_return(false)
  end

  describe '#etc_files' do
    it 'returns files at /etc/*' do
      expect(described_class.etc_files).to match_array(['/etc/file'])
    end
  end
end

# spec/recipes/some_recipe_spec.rb
it 'ensures files at /etc/* are owned by root' do
  expect(chef_run).to create_file('/etc/file').with(owner: 'root', group: 'root')
end

Config file rendering

Bad

# some_recipe.rb
template '/etc/some.conf' do
  source 'some.conf.erb'
  variables(user: node['some_conf']['user'], host: node['some_conf']['host'])
end

# templates/some.conf.erb
user = <%= user %>
host = <%= host %>

Good

# some_recipe.rb
template '/etc/some.conf' do
  source 'some.conf.erb'
end

# templates/some.conf.erb
<% node['some.conf'].each do |k, v| %>
<%= k %> = <%= v %>
<% end %>

Best

If the conf file has a library associated with it (conf.json, conf.yml, etc) then render the file from attributes without a template:

# some_recipe.rb
require 'json'
require 'yaml'

file '/etc/some_conf.json' do
  content node['some_conf'].to_json
end

file '/etc/some_conf.yml' do
  content node['some_conf'].to_yaml
end