Skip to content
Browse files

Simplify checks

* Separate checks from logic that collects item
  pairings for comparison
* Renamed checks to validations, failures to errors.
* Validations no longer blocks eval'd inside a
  CheckContext class.
* Validations are just methods prefixed with `validate_`
* Validations have access to a local item, server item
  and context object, as well as whatever helper methods
  needed.
* Reduced a lot of duplication
  • Loading branch information...
1 parent 6e5466a commit eed83403d125f89ded62e83f86e987b85b3d4bc7 @bmarini committed Oct 19, 2012
View
2 lib/health_inspector.rb
@@ -2,7 +2,7 @@
require "health_inspector/version"
require "health_inspector/color"
require "health_inspector/context"
-require "health_inspector/check"
+require "health_inspector/pairing"
require "health_inspector/inspector"
require "health_inspector/checklists/base"
require "health_inspector/checklists/cookbooks"
View
16 lib/health_inspector/check.rb
@@ -1,16 +0,0 @@
-module HealthInspector
- module Check
- def run_check
- check
- return failure
- end
-
- def failure(message=nil)
- if message
- @failure = message
- else
- @failure
- end
- end
- end
-end
View
53 lib/health_inspector/checklists/base.rb
@@ -7,12 +7,7 @@ class Base
include Color
class << self
- attr_reader :checks, :title
-
- def add_check(name, &block)
- @checks ||= []
- @checks << block
- end
+ attr_reader :title
def title(val=nil)
val.nil? ? @title : @title = val
@@ -39,7 +34,8 @@ def run
banner "Inspecting #{self.class.title}"
each_item do |item|
- failures = run_checks(item)
+ item.validate
+ failures = item.errors
if failures.empty?
print_success(item.name) unless @context.quiet_success
@@ -49,53 +45,10 @@ def run
end
end
- def run_checks(item)
- checks.map { |check| run_check(check, item) }.compact
- end
-
- def checks
- self.class.checks
- end
-
- class CheckContext
- include Check, Color
- attr_accessor :item, :context
-
- def initialize(check, item, context)
- @item = item
- @context = context
- @check = check
- end
-
- def call
- instance_eval(&@check)
- end
-
- def diff(original, other)
- (original.keys + other.keys).uniq.inject({}) do |memo, key|
- unless original[key] == other[key]
- if original[key].kind_of?(Hash) && other[key].kind_of?(Hash)
- memo[key] = diff(original[key], other[key])
- else
- memo[key] = {"server" => original[key],"local" => other[key]}
- end
- end
- memo
- end
- end
-
- end
-
def chef_rest
@context.chef_rest
end
- def run_check(check, item)
- check_context = CheckContext.new(check, item, @context)
- check_context.call
- return check_context.failure
- end
-
def banner(message)
puts
puts message
View
143 lib/health_inspector/checklists/cookbooks.rb
@@ -1,57 +1,94 @@
module HealthInspector
module Checklists
- class Cookbooks < Base
-
- add_check "local copy exists" do
- failure( "exists on server but not locally" ) if item.path.nil?
- end
+ class Cookbook < Pairing
+ include ExistenceValidations
- add_check "server copy exists" do
- failure( "exists locally but not on server" ) if item.server_version.nil?
- end
-
- add_check "versions" do
- if item.local_version && item.server_version &&
- item.local_version != item.server_version
- failure "chef server has #{item.server_version} but local version is #{item.local_version}"
+ def validate_versions
+ if versions_exist? && !versions_match?
+ errors.add "chef server has #{server} but local version is #{local}"
end
end
- add_check "uncommitted changes" do
- if item.git_repo?
- result = `cd #{item.path} && git status -s`
+ def validate_uncommited_changes
+ if git_repo?
+ result = `cd #{cookbook_path} && git status -s`
unless result.empty?
- failure "Uncommitted changes:\n#{result.chomp}"
+ errors.add "Uncommitted changes:\n#{result.chomp}"
end
end
end
- add_check "commits not pushed to remote" do
- if item.git_repo?
- result = `cd #{item.path} && git status`
+ def validate_commits_not_pushed_to_remote
+ if git_repo?
+ result = `cd #{cookbook_path} && git status`
if result =~ /Your branch is ahead of (.+)/
- failure "ahead of #{$1}"
+ errors.add "ahead of #{$1}"
end
end
end
- add_check "changes on the server not in the repo" do
- if item.server_version == item.local_version && !item.bad_files.empty?
- fail_message = "has a checksum mismatch between server and repo in\n"
- fail_message << item.bad_files.map { |f| " #{f}" }.join("\n")
- failure fail_message
+ # TODO: Check files that exist locally but not in manifest on server
+ def validate_changes_on_the_server_not_in_the_repo
+ if versions_exist? && versions_match?
+
+ begin
+ cookbook = context.chef_rest.get_rest("/cookbooks/#{name}/#{local}")
+ messages = []
+
+ Chef::CookbookVersion::COOKBOOK_SEGMENTS.each do |segment|
+ cookbook.manifest[segment].each do |manifest_record|
+ path = cookbook_path.join("#{manifest_record["path"]}")
+
+ if path.exist?
+ checksum = checksum_cookbook_file(path)
+ messages << "#{manifest_record['path']}" if checksum != manifest_record['checksum']
+ else
+ messages << "#{manifest_record['path']} does not exist in the repo"
+ end
+ end
+ end
+
+ unless messages.empty?
+ message = "has a checksum mismatch between server and repo in\n"
+ message << messages.map { |f| " #{f}" }.join("\n")
+ errors.add message
+ end
+
+ rescue Net::HTTPServerException => e
+ errors.add "Could not find cookbook #{name} on the server"
+ end
+
end
end
- class Cookbook < Struct.new(:name, :path, :server_version, :local_version, :bad_files)
- def git_repo?
- self.path && File.exist?("#{self.path}/.git")
- end
+ def versions_exist?
+ local && server
+ end
+
+ def versions_match?
+ local == server
+ end
+
+ def git_repo?
+ cookbook_path && File.exist?("#{cookbook_path}/.git")
+ end
+
+ def cookbook_path
+ path = context.cookbook_path.find { |f| File.exist?("#{f}/#{name}") }
+ path ? Pathname.new(path).join(name) : nil
+ end
+
+ def checksum_cookbook_file(filepath)
+ Chef::CookbookVersion.checksum_cookbook_file(filepath)
end
+ end
+
+ class Cookbooks < Base
+
title "cookbooks"
def each_item
@@ -60,13 +97,11 @@ def each_item
all_cookbook_names = ( server_cookbooks.keys + local_cookbooks.keys ).uniq.sort
all_cookbook_names.each do |name|
- item = Cookbook.new.tap do |cookbook|
- cookbook.name = name
- cookbook.path = cookbook_path(name)
- cookbook.server_version = server_cookbooks[name]
- cookbook.local_version = local_cookbooks[name]
- cookbook.bad_files = checksum_compare(name, cookbook.server_version.inspect)
- end
+ item = Cookbook.new(@context,
+ :name => name,
+ :server => server_cookbooks[name],
+ :local => local_cookbooks[name]
+ )
yield item
end
@@ -94,40 +129,6 @@ def cookbooks_in_repo
end
end
- def cookbook_path(name)
- path = @context.cookbook_path.find { |f| File.exist?("#{f}/#{name}") }
- path ? File.join(path, name) : nil
- end
-
- # TODO: Check files that exist locally but not in manifest on server
- def checksum_compare(name, version)
- begin
- cookbook = chef_rest.get_rest("/cookbooks/#{name}/#{version}")
- rescue Net::HTTPServerException => e
- return ["Could not find cookbook #{name} on the server"]
- end
-
- bad_files = []
-
- Chef::CookbookVersion::COOKBOOK_SEGMENTS.each do |segment|
- cookbook.manifest[segment].each do |manifest_record|
- path = cookbook_path("#{name}/#{manifest_record["path"]}")
-
- if path
- checksum = checksum_cookbook_file(path)
- bad_files << "#{manifest_record['path']}" if checksum != manifest_record['checksum']
- else
- bad_files << "#{manifest_record['path']} does not exist in the repo"
- end
- end
- end
-
- bad_files
- end
-
- def checksum_cookbook_file(filepath)
- Chef::CookbookVersion.checksum_cookbook_file(filepath)
- end
end
end
end
View
32 lib/health_inspector/checklists/data_bag_items.rb
@@ -2,37 +2,25 @@
module HealthInspector
module Checklists
+ class DataBagItem < Pairing
+ include ExistenceValidations
+ include JsonValidations
+ end
+
class DataBagItems < Base
title "data bag items"
- add_check "local copy exists" do
- failure "exists on server but not locally" unless item.local
- end
-
- add_check "server copy exists" do
- failure "exists locally but not on server" unless item.server
- end
-
- add_check "items are the same" do
- if item.server && item.local
- item_diff = diff(item.server, item.local)
- failure item_diff unless item_diff.empty?
- end
- end
-
- DataBagItem = Struct.new(:name, :server, :local)
-
def each_item
server_data_bag_items = data_bag_items_on_server
local_data_bag_items = data_bag_items_in_repo
all_data_bag_item_names = ( server_data_bag_items + local_data_bag_items ).uniq.sort
all_data_bag_item_names.each do |name|
- item = DataBagItem.new.tap do |data_bag_item|
- data_bag_item.name = name
- data_bag_item.server = load_item_from_server(name)
- data_bag_item.local = load_item_from_local(name)
- end
+ item = DataBagItem.new(@context,
+ :name => name,
+ :server => load_item_from_server(name),
+ :local => load_item_from_local(name)
+ )
yield item
end
View
24 lib/health_inspector/checklists/data_bags.rb
@@ -2,30 +2,24 @@
module HealthInspector
module Checklists
+ class DataBag < Pairing
+ include ExistenceValidations
+ end
+
class DataBags < Base
title "data bags"
- add_check "local copy exists" do
- failure "exists on server but not locally" unless item.exists_locally
- end
-
- add_check "server copy exists" do
- failure "exists locally but not on server" unless item.exists_on_server
- end
-
- DataBag = Struct.new(:name, :exists_on_server, :exists_locally)
-
def each_item
server_data_bags = data_bags_on_server
local_data_bags = data_bags_in_repo
all_data_bag_names = ( server_data_bags + local_data_bags ).uniq.sort
all_data_bag_names.each do |name|
- item = DataBag.new.tap do |data_bag|
- data_bag.name = name
- data_bag.exists_on_server = data_bags_on_server.include?(name)
- data_bag.exists_locally = data_bags_in_repo.include?(name)
- end
+ item = DataBag.new(@context,
+ :name => name,
+ :server => data_bags_on_server.include?(name),
+ :local => data_bags_in_repo.include?(name)
+ )
yield item
end
View
35 lib/health_inspector/checklists/environments.rb
@@ -2,37 +2,30 @@
module HealthInspector
module Checklists
- class Environments < Base
- title "environments"
-
- add_check "local copy exists" do
- failure "exists on server but not locally" unless item.local || item.name == '_default'
- end
-
- add_check "server copy exists" do
- failure "exists locally but not on server" unless item.server
- end
+ class Environment < Pairing
+ include ExistenceValidations
+ include JsonValidations
- add_check "items are the same" do
- if item.server && item.local
- item_diff = diff(item.server, item.local)
- failure item_diff unless item_diff.empty?
- end
+ # Override to ignore _default environment if it is missing locally
+ def validate_local_copy_exists
+ super unless name == '_default'
end
+ end
- Environment = Struct.new(:name, :server, :local)
+ class Environments < Base
+ title "environments"
def each_item
server_items = items_on_server
local_items = items_in_repo
all_item_names = ( server_items + local_items ).uniq.sort
all_item_names.each do |name|
- item = Environment.new.tap do |item|
- item.name = name
- item.server = load_item_from_server(name)
- item.local = load_item_from_local(name)
- end
+ item = Environment.new(@context,
+ :name => name,
+ :server => load_item_from_server(name),
+ :local => load_item_from_local(name)
+ )
yield item
end
View
35 lib/health_inspector/checklists/roles.rb
@@ -1,40 +1,27 @@
require "chef/role"
+require 'yajl'
module HealthInspector
module Checklists
+ class Role < Pairing
+ include ExistenceValidations
+ include JsonValidations
+ end
+
class Roles < Base
title "roles"
- require 'yajl'
-
- add_check "local copy exists" do
- failure "exists on server but not locally" unless item.local
- end
-
- add_check "server copy exists" do
- failure "exists locally but not on server" unless item.server
- end
-
- add_check "items are the same" do
- if item.server && item.local
- item_diff = diff(item.server, item.local)
- failure item_diff unless item_diff.empty?
- end
- end
-
- Role = Struct.new(:name, :server, :local)
-
def each_item
server_items = items_on_server
local_items = items_in_repo
all_item_names = ( server_items + local_items ).uniq.sort
all_item_names.each do |name|
- item = Role.new.tap do |item|
- item.name = name
- item.server = load_item_from_server(name)
- item.local = load_item_from_local(name)
- end
+ item = Role.new(@context,
+ :name => name,
+ :server => load_item_from_server(name),
+ :local => load_item_from_local(name)
+ )
yield item
end
View
73 lib/health_inspector/pairing.rb
@@ -0,0 +1,73 @@
+module HealthInspector
+ class Errors
+ include Enumerable
+
+ def initialize
+ @errors = []
+ end
+
+ def add(message)
+ @errors << message
+ end
+
+ def each
+ @errors.each { |e| yield(e) }
+ end
+
+ def empty?
+ @errors.empty?
+ end
+ end
+
+ class Pairing
+ attr_accessor :name, :local, :server
+ attr_reader :context, :errors
+
+ def initialize(context, opts={})
+ @context = context
+ @name = opts[:name]
+ @local = opts[:local]
+ @server = opts[:server]
+
+ @validations = []
+ @errors = Errors.new
+ end
+
+ def validate
+ self.methods.grep(/^validate_/).each { |meth| send(meth) }
+ end
+
+ def hash_diff(original, other)
+ (original.keys + other.keys).uniq.inject({}) do |memo, key|
+ unless original[key] == other[key]
+ if original[key].kind_of?(Hash) && other[key].kind_of?(Hash)
+ memo[key] = diff(original[key], other[key])
+ else
+ memo[key] = {"server" => original[key],"local" => other[key]}
+ end
+ end
+ memo
+ end
+ end
+ end
+
+ # Mixins for common validations across pairings
+ module ExistenceValidations
+ def validate_local_copy_exists
+ errors.add "exists on server but not locally" if local.nil?
+ end
+
+ def validate_server_copy_exists
+ errors.add "exists locally but not on server" if server.nil?
+ end
+ end
+
+ module JsonValidations
+ def validate_items_are_the_same
+ if server && local
+ differences = hash_diff(server, local)
+ errors.add differences unless differences.empty?
+ end
+ end
+ end
+end
View
45 spec/cookbook_spec.rb
@@ -1,26 +1,41 @@
require 'spec_helper'
-describe "HealthInspector::Checklists::Cookbooks" do
- subject do
- HealthInspector::Checklists::Cookbooks.new(health_inspector_context)
+describe HealthInspector::Checklists::Cookbook do
+ let(:pairing) { described_class.new(health_inspector_context, :name => "dummy") }
+
+ it "should detect if an item does not exist locally" do
+ pairing.server = "0.0.1"
+ pairing.local = nil
+ pairing.validate
+
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == "exists on server but not locally"
end
- # :name, :path, :server_version, :local_version, :bad_files
- let(:item) { HealthInspector::Checklists::Cookbooks::Cookbook }
+ it "should detect if an item does not exist on server" do
+ pairing.server = nil
+ pairing.local = "0.0.1"
+ pairing.validate
+
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == "exists locally but not on server"
+ end
- it "should detect if a cookbook does not exist locally" do
- obj = item.new("ruby", nil, "0.0.1", nil, [])
+ it "should detect if an item is different" do
+ pairing.server = "0.0.1"
+ pairing.local = "0.0.2"
+ pairing.validate
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists on server but not locally"
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == "chef server has 0.0.1 but local version is 0.0.2"
end
- it "should detect if a cookbook does not exist on server" do
- obj = item.new("ruby", "cookbooks/ruby", nil, "0.0.1", [])
+ it "should detect if an item is the same" do
+ pairing.should_receive(:validate_changes_on_the_server_not_in_the_repo)
+ pairing.server = "0.0.1"
+ pairing.local = "0.0.1"
+ pairing.validate
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists locally but not on server"
+ pairing.errors.should be_empty
end
end
View
42 spec/data_bag_item_spec.rb
@@ -1,40 +1,6 @@
require 'spec_helper'
-describe "HealthInspector::Checklists::DataBagItems" do
- subject do
- HealthInspector::Checklists::DataBagItems.new(health_inspector_context)
- end
-
- let(:item) { HealthInspector::Checklists::DataBagItems::DataBagItem }
-
- it "should detect if a data bag item does not exist locally" do
- obj = item.new("apps", {"foo" => "bar"}, nil)
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists on server but not locally"
- end
-
- it "should detect if a data bag item does not exist on server" do
- obj = item.new("apps", nil, {"foo" => "bar"})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists locally but not on server"
- end
-
- it "should detect if a data bag item is different" do
- obj = item.new("apps", {"foo" => "bar"}, {"foo" => "baz"})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == {"foo" => {"server" => "bar", "local" => "baz"}}
- end
-
- it "should detect if a data bag item is the same" do
- obj = item.new("apps", {"foo" => "bar"}, {"foo" => "bar"})
-
- failures = subject.run_checks(obj)
- failures.should be_empty
- end
-end
+describe HealthInspector::Checklists::DataBagItem do
+ it_behaves_like "a chef model"
+ it_behaves_like "a chef model that can be respresented in json"
+end
View
24 spec/data_bag_spec.rb
@@ -1,25 +1,5 @@
require 'spec_helper'
-describe "HealthInspector::Checklists::DataBags" do
- subject do
- HealthInspector::Checklists::DataBags.new(health_inspector_context)
- end
-
- let(:item) { HealthInspector::Checklists::DataBags::DataBag }
-
- it "should detect if a data bag does not exist locally" do
- obj = item.new("users", true, false)
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists on server but not locally"
- end
-
- it "should detect if a data bag does not exist on server" do
- obj = item.new("users", false, true)
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists locally but not on server"
- end
+describe HealthInspector::Checklists::DataBag do
+ it_behaves_like "a chef model"
end
View
51 spec/environment_spec.rb
@@ -1,47 +1,18 @@
require 'spec_helper'
-describe "HealthInspector::Checklists::Environments" do
- subject do
- HealthInspector::Checklists::Environments.new(health_inspector_context)
- end
-
- let(:item) { HealthInspector::Checklists::Environments::Environment }
-
- it "should detect if an environment does not exist locally" do
- obj = item.new("production", {}, nil)
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists on server but not locally"
- end
-
- it "should detect if an environment does not exist on server" do
- obj = item.new("production", nil, {})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists locally but not on server"
- end
-
- it "should detect if an environment is different" do
- obj = item.new("production", {"foo" => "bar"}, {"foo" => "baz"})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == {"foo"=>{"server"=>"bar", "local"=>"baz"}}
- end
+describe HealthInspector::Checklists::Environment do
+ let(:pairing) { described_class.new(health_inspector_context) }
- it "should detect if an environment is the same" do
- obj = item.new("production", {}, {})
-
- failures = subject.run_checks(obj)
- failures.should be_empty
- end
+ it_behaves_like "a chef model"
+ it_behaves_like "a chef model that can be respresented in json"
it "should ignore _default environment if it only exists on server" do
- obj = item.new("_default", {}, nil)
+ pairing.name = "_default"
+ pairing.server = {}
+ pairing.local = nil
+ pairing.validate
- failures = subject.run_checks(obj)
- failures.should be_empty
+ pairing.errors.should be_empty
end
-end
+
+end
View
43 spec/role_spec.rb
@@ -1,41 +1,6 @@
require 'spec_helper'
-describe "HealthInspector::Checklists::Roles" do
- subject do
- HealthInspector::Checklists::Roles.new(health_inspector_context)
- end
-
- let(:item) { HealthInspector::Checklists::Roles::Role }
-
- it "should detect if an role does not exist locally" do
- obj = item.new("app-server", {}, nil)
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists on server but not locally"
- end
-
- it "should detect if an role does not exist on server" do
- obj = item.new("app-server", nil, {})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == "exists locally but not on server"
- end
-
- it "should detect if an role is different" do
- obj = item.new("app-server", {"foo" => "bar"}, {"foo" => "baz"})
-
- failures = subject.run_checks(obj)
- failures.should_not be_empty
- failures.first.should == {"foo"=>{"server"=>"bar", "local"=>"baz"}}
- end
-
- it "should detect if an role is the same" do
- obj = item.new("app-server", {}, {})
-
- failures = subject.run_checks(obj)
- failures.should be_empty
- end
-
-end
+describe HealthInspector::Checklists::Role do
+ it_behaves_like "a chef model"
+ it_behaves_like "a chef model that can be respresented in json"
+end
View
43 spec/spec_helper.rb
@@ -16,4 +16,47 @@ def health_inspector_context
RSpec.configure do |c|
c.include HealthInspector::SpecHelpers
+end
+
+shared_examples "a chef model" do
+ let(:pairing) { described_class.new(health_inspector_context, :name => "dummy") }
+
+ it "should detect if an item does not exist locally" do
+ pairing.server = {}
+ pairing.local = nil
+ pairing.validate
+
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == "exists on server but not locally"
+ end
+
+ it "should detect if an item does not exist on server" do
+ pairing.server = nil
+ pairing.local = {}
+ pairing.validate
+
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == "exists locally but not on server"
+ end
+end
+
+shared_examples "a chef model that can be respresented in json" do
+ let(:pairing) { described_class.new(health_inspector_context, :name => "dummy") }
+
+ it "should detect if an item is different" do
+ pairing.server = {"foo" => "bar"}
+ pairing.local = {"foo" => "baz"}
+ pairing.validate
+
+ pairing.errors.should_not be_empty
+ pairing.errors.first.should == {"foo"=>{"server"=>"bar", "local"=>"baz"}}
+ end
+
+ it "should detect if an item is the same" do
+ pairing.server = {"foo" => "bar"}
+ pairing.local = {"foo" => "bar"}
+ pairing.validate
+
+ pairing.errors.should be_empty
+ end
end

0 comments on commit eed8340

Please sign in to comment.
Something went wrong with that request. Please try again.