Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Greatly improved error recovery & reporting in Kitchen::Loader::YAML.

Now on error when parsing YAML config files or processing ERB, a more
helpful message is displayed to the user, instructing them to run
`kitchen diagnose --no-instances --loader` for more details.

More care was taken to handle specific file loading errors, and a
special case when an entire YAML file has been commented out with hash
characters at the beggining of lines.

Additionally, full spec coverage is in place and ready for future bug
and regression cases.

Closes #285
  • Loading branch information...
commit 1c924af2e9f6ef55f5706bd432d58928130533e7 1 parent 2a1ffc2
@fnichol fnichol authored
Showing with 312 additions and 23 deletions.
  1. +42 −21 lib/kitchen/loader/yaml.rb
  2. +270 −2 spec/kitchen/loader/yaml_spec.rb
View
63 lib/kitchen/loader/yaml.rb
@@ -67,21 +67,13 @@ def read
# @return [Hash] a diagnostic hash
def diagnose
result = Hash.new
- result[:proces_erb] = @process_erb
+ result[:process_erb] = @process_erb
result[:process_local] = @process_local
result[:process_global] = @process_global
- if File.exists?(global_config_file)
- result[:global_config] =
- { :filename => global_config_file, :raw_data => global_yaml }
- end
- result[:project_config] =
- { :filename => config_file, :raw_data => yaml }
- if File.exists?(local_config_file)
- result[:local_config] =
- { :filename => local_config_file, :raw_data => local_yaml }
- end
- combined = begin ; combined_hash ; rescue => e ; failure_hash(e) ; end
- result[:combined_config] = { :raw_data => combined }
+ result[:global_config] = diagnose_component(:global_yaml, global_config_file)
+ result[:project_config] = diagnose_component(:yaml, config_file)
+ result[:local_config] = diagnose_component(:local_yaml, local_config_file)
+ result[:combined_config] = diagnose_component(:combined_hash)
result
end
@@ -98,9 +90,6 @@ def combined_hash
normalize(yaml)
end
@process_global ? y.rmerge(normalize(global_yaml)) : y
- rescue NoMethodError
- raise UserError, "Error merging #{File.basename(config_file)} and" +
- "#{File.basename(local_config_file)}"
end
def yaml
@@ -118,7 +107,16 @@ def global_yaml
def yaml_string(file)
string = read_file(file)
- @process_erb ? ERB.new(string).result : string
+ @process_erb ? process_erb(string, file) : string
+ end
+
+ def process_erb(string, file)
+ ERB.new(string).result
+ rescue => e
+ raise UserError, "Error parsing ERB content in #{file} " +
+ "(#{e.class}: #{e.message}).\n" +
+ "Please run `kitchen diagnose --no-instances --loader' to help " +
+ "debug your issue."
end
def read_file(file)
@@ -133,14 +131,28 @@ def global_config_file
File.join(File.expand_path(ENV["HOME"]), ".kitchen", "config.yml")
end
- def failure_hash(e)
- {
+ def diagnose_component(component, file = nil)
+ return if file && !File.exists?(file)
+
+ hash = begin
+ send(component)
+ rescue => e
+ failure_hash(e, file)
+ end
+
+ { :filename => file, :raw_data => hash }
+ end
+
+ def failure_hash(e, file = nil)
+ result = {
:error => {
:exception => e.inspect,
:message => e.message,
:backtrace => e.backtrace
}
}
+ result[:error][:raw_file] = IO.read(file) unless file.nil?
+ result
end
def normalize(obj)
@@ -170,9 +182,18 @@ def normalize_hash(hash, key, value)
def parse_yaml_string(string, file_name)
return Hash.new if string.nil? || string.empty?
- ::YAML.safe_load(string) || Hash.new
+ result = ::YAML.safe_load(string) || Hash.new
+ unless result.is_a?(Hash)
+ raise UserError, "Error parsing #{file_name} as YAML " +
+ "(Result of parse was not a Hash, but was a #{result.class}).\n" +
+ "Please run `kitchen diagnose --no-instances --loader' to help " +
+ "debug your issue."
+ end
+ result
rescue SyntaxError, Psych::SyntaxError
- raise UserError, "Error parsing #{file_name}"
+ raise UserError, "Error parsing #{file_name} as YAML.\n" +
+ "Please run `kitchen diagnose --no-instances --loader' to help " +
+ "debug your issue."
end
end
end
View
272 spec/kitchen/loader/yaml_spec.rb
@@ -326,14 +326,28 @@ class Yamled
FileUtils.mkdir_p "/tmp"
File.open("/tmp/.kitchen.yml", "wb") { |f| f.write '&*%^*' }
- proc { loader.read }.must_raise Kitchen::UserError
+ err = proc { loader.read }.must_raise Kitchen::UserError
+ err.message.must_match Regexp.new(Regexp.escape(
+ "Error parsing /tmp/.kitchen.yml"))
end
it "raises a UserError if kitchen.yml cannot be parsed" do
FileUtils.mkdir_p "/tmp"
File.open("/tmp/.kitchen.yml", "wb") { |f| f.write 'uhoh' }
- proc { loader.read }.must_raise Kitchen::UserError
+ err = proc { loader.read }.must_raise Kitchen::UserError
+ err.message.must_match Regexp.new(Regexp.escape(
+ "Error parsing /tmp/.kitchen.yml"))
+ end
+
+ it "raises a UserError if kitchen.yml is a commented out YAML document" do
+ FileUtils.mkdir_p "/tmp"
+ # this is not technically valid YAML and worse yet returns a Psych::Paser
+ File.open("/tmp/.kitchen.yml", "wb") { |f| f.write '#---\n' }
+
+ err = proc { loader.read }.must_raise Kitchen::UserError
+ err.message.must_match Regexp.new(Regexp.escape(
+ "Error parsing /tmp/.kitchen.yml"))
end
it "raises a UserError if kitchen.local.yml cannot be parsed" do
@@ -356,6 +370,20 @@ class Yamled
loader.read.must_equal({ :name => "ahhchoo" })
end
+ it "raises a UserError if there is an ERB processing error" do
+ FileUtils.mkdir_p "/tmp"
+ File.open("/tmp/.kitchen.yml", "wb") do |f|
+ f.write <<-'YAML'.gsub(/^ {10}/, '')
+ ---
+ <%= poop %>: yep
+ YAML
+ end
+
+ err = proc { loader.read }.must_raise Kitchen::UserError
+ err.message.must_match Regexp.new(Regexp.escape(
+ "Error parsing ERB content in /tmp/.kitchen.yml"))
+ end
+
it "evaluates kitchen.local.yml through erb before loading by default" do
FileUtils.mkdir_p "/tmp"
File.open("/tmp/.kitchen.local.yml", "wb") do |f|
@@ -431,6 +459,246 @@ class Yamled
end
end
+ describe "#diagnose" do
+
+ it "returns a Hash" do
+ stub_yaml!(Hash.new)
+
+ loader.diagnose.must_be_kind_of(Hash)
+ end
+
+ it "contains erb processing information when true" do
+ stub_yaml!(Hash.new)
+
+ loader.diagnose[:process_erb].must_equal true
+ end
+
+ it "contains erb processing information when false" do
+ stub_yaml!(Hash.new)
+ loader = Kitchen::Loader::YAML.new(
+ '/tmp/.kitchen.yml', :process_erb => false)
+
+ loader.diagnose[:process_erb].must_equal false
+ end
+
+ it "contains local processing information when true" do
+ stub_yaml!(Hash.new)
+
+ loader.diagnose[:process_local].must_equal true
+ end
+
+ it "contains local processing information when false" do
+ stub_yaml!(Hash.new)
+ loader = Kitchen::Loader::YAML.new(
+ '/tmp/.kitchen.yml', :process_local => false)
+
+ loader.diagnose[:process_local].must_equal false
+ end
+
+ it "contains global processing information when true" do
+ stub_yaml!(Hash.new)
+
+ loader.diagnose[:process_global].must_equal true
+ end
+
+ it "contains global processing information when false" do
+ stub_yaml!(Hash.new)
+ loader = Kitchen::Loader::YAML.new(
+ '/tmp/.kitchen.yml', :process_global => false)
+
+ loader.diagnose[:process_global].must_equal false
+ end
+
+ describe "for yaml files" do
+
+ before do
+ stub_yaml!(".kitchen.yml", {
+ "from_project" => "project",
+ "common" => { "p" => "pretty" }
+ })
+ stub_yaml!(".kitchen.local.yml", {
+ "from_local" => "local",
+ "common" => { "l" => "looky" }
+ })
+ stub_global!({
+ "from_global" => "global",
+ "common" => { "g" => "goody" }
+ })
+ end
+
+ it "global config contains a filename" do
+ loader.diagnose[:global_config][:filename].
+ must_equal File.join(ENV["HOME"], ".kitchen/config.yml")
+ end
+
+ it "global config contains raw data" do
+ loader.diagnose[:global_config][:raw_data].must_equal({
+ "from_global" => "global",
+ "common" => { "g" => "goody" }
+ })
+ end
+
+ it "project config contains a filename" do
+ loader.diagnose[:project_config][:filename].
+ must_equal "/tmp/.kitchen.yml"
+ end
+
+ it "project config contains raw data" do
+ loader.diagnose[:project_config][:raw_data].must_equal({
+ "from_project" => "project",
+ "common" => { "p" => "pretty" }
+ })
+ end
+
+ it "local config contains a filename" do
+ loader.diagnose[:local_config][:filename].
+ must_equal "/tmp/.kitchen.local.yml"
+ end
+
+ it "local config contains raw data" do
+ loader.diagnose[:local_config][:raw_data].must_equal({
+ "from_local" => "local",
+ "common" => { "l" => "looky" }
+ })
+ end
+
+ it "combined config contains a nil filename" do
+ loader.diagnose[:combined_config][:filename].
+ must_equal nil
+ end
+
+ it "combined config contains raw data" do
+ loader.diagnose[:combined_config][:raw_data].must_equal({
+ "from_global" => "global",
+ "from_project" => "project",
+ "from_local" => "local",
+ "common" => {
+ "g" => "goody",
+ "p" => "pretty",
+ "l" => "looky"
+ }
+ })
+ end
+
+ describe "for global on error" do
+
+ before do
+ FileUtils.mkdir_p(File.join(ENV["HOME"], ".kitchen"))
+ File.open(File.join(ENV["HOME"], ".kitchen/config.yml"), "wb") do |f|
+ f.write '#---\n'
+ end
+ end
+
+ it "uses an error hash with the raw file contents" do
+ loader.diagnose[:global_config][:raw_data][:error][:raw_file].
+ must_equal "#---\\n"
+ end
+
+ it "uses an error hash with the exception" do
+ loader.diagnose[:global_config][:raw_data][:error][:exception].
+ must_match %r{Kitchen::UserError}
+ end
+
+ it "uses an error hash with the exception message" do
+ loader.diagnose[:global_config][:raw_data][:error][:message].
+ must_match %r{Error parsing}
+ end
+
+ it "uses an error hash with the exception backtrace" do
+ loader.diagnose[:global_config][:raw_data][:error][:backtrace].
+ must_be_kind_of Array
+ end
+ end
+
+ describe "for project on error" do
+
+ before do
+ File.open("/tmp/.kitchen.yml", "wb") do |f|
+ f.write '#---\n'
+ end
+ end
+
+ it "uses an error hash with the raw file contents" do
+ loader.diagnose[:project_config][:raw_data][:error][:raw_file].
+ must_equal "#---\\n"
+ end
+
+ it "uses an error hash with the exception" do
+ loader.diagnose[:project_config][:raw_data][:error][:exception].
+ must_match %r{Kitchen::UserError}
+ end
+
+ it "uses an error hash with the exception message" do
+ loader.diagnose[:project_config][:raw_data][:error][:message].
+ must_match %r{Error parsing}
+ end
+
+ it "uses an error hash with the exception backtrace" do
+ loader.diagnose[:project_config][:raw_data][:error][:backtrace].
+ must_be_kind_of Array
+ end
+ end
+
+ describe "for local on error" do
+
+ before do
+ File.open("/tmp/.kitchen.local.yml", "wb") do |f|
+ f.write '#---\n'
+ end
+ end
+
+ it "uses an error hash with the raw file contents" do
+ loader.diagnose[:local_config][:raw_data][:error][:raw_file].
+ must_equal "#---\\n"
+ end
+
+ it "uses an error hash with the exception" do
+ loader.diagnose[:local_config][:raw_data][:error][:exception].
+ must_match %r{Kitchen::UserError}
+ end
+
+ it "uses an error hash with the exception message" do
+ loader.diagnose[:local_config][:raw_data][:error][:message].
+ must_match %r{Error parsing}
+ end
+
+ it "uses an error hash with the exception backtrace" do
+ loader.diagnose[:local_config][:raw_data][:error][:backtrace].
+ must_be_kind_of Array
+ end
+ end
+
+ describe "for combined on error" do
+
+ before do
+ File.open("/tmp/.kitchen.yml", "wb") do |f|
+ f.write '#---\n'
+ end
+ end
+
+ it "uses an error hash with nil raw file contents" do
+ loader.diagnose[:combined_config][:raw_data][:error][:raw_file].
+ must_equal nil
+ end
+
+ it "uses an error hash with the exception" do
+ loader.diagnose[:combined_config][:raw_data][:error][:exception].
+ must_match %r{Kitchen::UserError}
+ end
+
+ it "uses an error hash with the exception message" do
+ loader.diagnose[:combined_config][:raw_data][:error][:message].
+ must_match %r{Error parsing}
+ end
+
+ it "uses an error hash with the exception backtrace" do
+ loader.diagnose[:combined_config][:raw_data][:error][:backtrace].
+ must_be_kind_of Array
+ end
+ end
+ end
+ end
+
private
def stub_file(path, hash)

1 comment on commit 1c924af

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