Ignore non-critical errors when finding the cookbook set

https://tickets.opscode.com/browse/CHEF-2923

When running `knife cookbook upload SPECIFIC_COOKBOOK`, errors in
metadata files for unrelated cookbooks should not be raised; however, we
must evaluate the metadata for all cookbooks in order to allow the
metadata `name` attribute to be different from the cookbook directory's
basename. Therefore, we must tolerate errors in metadata.rb and re-raise
them only when attempting to load the cookbook with the invalid
metadata.

6 files changed, 106 insertions, 10 deletions

diff --git a/lib/chef/cookbook/cookbook_version_loader.rb b/lib/chef/cookbook/cookbook_version_loader.rb
index 745a5279ef..02bc4fb0db 100644
--- a/lib/chef/cookbook/cookbook_version_loader.rb
+++ b/lib/chef/cookbook/cookbook_version_loader.rb
@@ -29,6 +29,8 @@ class Chef
       # The cookbook's name as inferred from its directory.
       attr_reader :inferred_cookbook_name
 
+      attr_reader :metadata_error
+
       def initialize(path, chefignore=nil)
         @cookbook_path = File.expand_path( path ) # cookbook_path from which this was loaded
         # We keep a list of all cookbook paths that have been merged in
@@ -53,8 +55,11 @@ class Chef
         }
 
         @metadata_filenames = []
+        @metadata_error = nil
       end
 
+      # Load the cookbook. Raises an error if the cookbook_path given to the
+      # constructor doesn't point to a valid cookbook.
       def load!
         file_paths_map = load
 
@@ -64,7 +69,15 @@ class Chef
         file_paths_map
       end
 
+      # Load the cookbook. Does not raise an error if given a non-cookbook
+      # directory as the cookbook_path. This behavior is provided for
+      # compatibility, it is recommended to use #load! instead.
       def load
+        metadata # force lazy evaluation to occur
+
+        # re-raise any exception that occurred when reading the metadata
+        raise_metadata_error!
+
         load_as(:attribute_filenames, 'attributes', '*.rb')
         load_as(:definition_filenames, 'definitions', '*.rb')
         load_as(:recipe_filenames, 'recipes', '*.rb')
@@ -134,6 +147,12 @@ class Chef
 
         @metadata = Chef::Cookbook::Metadata.new
 
+        # Compatibility if metadata is missing the name attribute:
+        # TODO: Metadata should distinguish between inferred name and actual.
+        if @metadata.name.nil?
+          @metadata.name(@inferred_cookbook_name)
+        end
+
         metadata_filenames.each do |metadata_file|
           case metadata_file
           when /\.rb$/
@@ -147,13 +166,21 @@ class Chef
           end
         end
 
-        # Compatibility if metadata is missing the name attribute:
-        # TODO: probably should live elsewhere.
-        if @metadata.name.nil?
-          @metadata.name(@inferred_cookbook_name)
-        end
 
         @metadata
+
+        # Rescue errors so that users can upload cookbooks via `knife cookbook
+        # upload` even if some cookbooks in their chef-repo have errors in
+        # their metadata. We only rescue StandardError because you have to be
+        # doing something *really* terrible to raise an exception that inherits
+        # directly from Exception in your metadata.rb file.
+      rescue StandardError => e
+        @metadata_error = e
+        @metadata
+      end
+
+      def raise_metadata_error!
+        raise @metadata_error unless @metadata_error.nil?
       end
 
       def empty?
diff --git a/spec/data/invalid-metadata-chef-repo/invalid-metadata/README.md b/spec/data/invalid-metadata-chef-repo/invalid-metadata/README.md
new file mode 100644
index 0000000000..a41867f17c
--- /dev/null
+++ b/spec/data/invalid-metadata-chef-repo/invalid-metadata/README.md
@@ -0,0 +1,4 @@
+# invalid-metadata
+
+TODO: Enter the cookbook description here.
+
diff --git a/spec/data/invalid-metadata-chef-repo/invalid-metadata/metadata.rb b/spec/data/invalid-metadata-chef-repo/invalid-metadata/metadata.rb
new file mode 100644
index 0000000000..f548ad1dd8
--- /dev/null
+++ b/spec/data/invalid-metadata-chef-repo/invalid-metadata/metadata.rb
@@ -0,0 +1,10 @@
+raise "THIS METADATA HAS A BUG"
+
+name             'invalid-metadata'
+maintainer       ''
+maintainer_email ''
+license          ''
+description      'Installs/Configures invalid-metadata'
+long_description 'Installs/Configures invalid-metadata'
+version          '0.1.0'
+
diff --git a/spec/data/invalid-metadata-chef-repo/invalid-metadata/recipes/default.rb b/spec/data/invalid-metadata-chef-repo/invalid-metadata/recipes/default.rb
new file mode 100644
index 0000000000..1411b9e39f
--- /dev/null
+++ b/spec/data/invalid-metadata-chef-repo/invalid-metadata/recipes/default.rb
@@ -0,0 +1,8 @@
+#
+# Cookbook Name:: invalid-metadata
+# Recipe:: default
+#
+# Copyright (C) 2014 
+#
+# 
+#
diff --git a/spec/unit/cookbook/cookbook_version_loader_spec.rb b/spec/unit/cookbook/cookbook_version_loader_spec.rb
index 9528eea198..c8292de762 100644
--- a/spec/unit/cookbook/cookbook_version_loader_spec.rb
+++ b/spec/unit/cookbook/cookbook_version_loader_spec.rb
@@ -110,16 +110,28 @@ describe Chef::Cookbook::CookbookVersionLoader do
 
     context "when a cookbook has an invalid metadata file [CHEF-2923]" do
 
+      let(:cookbook_path) { File.join(CHEF_SPEC_DATA, "invalid-metadata-chef-repo/invalid-metadata") }
+
       it "raises an error when loading with #load!" do
-        pending
+        expect { cookbook_loader.load! }.to raise_error("THIS METADATA HAS A BUG")
       end
 
-      it "skips the cookbook when called with #load" do
-        pending
+      it "raises an error when called with #load" do
+        expect { cookbook_loader.load }.to raise_error("THIS METADATA HAS A BUG")
+      end
+
+      it "doesn't raise an error when determining the cookbook name" do
+        expect { cookbook_loader.cookbook_name }.to_not raise_error
+      end
+
+      it "doesn't raise an error when metadata is first generated" do
+        expect { cookbook_loader.metadata }.to_not raise_error
       end
 
       it "sets an error flag containing error information" do
-        pending
+        cookbook_loader.metadata
+        expect(cookbook_loader.metadata_error).to be_a(StandardError)
+        expect(cookbook_loader.metadata_error.message).to eq("THIS METADATA HAS A BUG")
       end
 
     end
diff --git a/spec/unit/cookbook_loader_spec.rb b/spec/unit/cookbook_loader_spec.rb
index 8c1a0886c2..d5d585b8e1 100644
--- a/spec/unit/cookbook_loader_spec.rb
+++ b/spec/unit/cookbook_loader_spec.rb
@@ -167,10 +167,26 @@ describe Chef::CookbookLoader do
         cookbook_loader.metadata[:openldap].should be_a_kind_of(Chef::Cookbook::Metadata)
       end
 
-    end # after loading cookbooks
+    end # referencing cookbook files
 
   end # loading all cookbooks
 
+  context "loading all cookbooks when one has invalid metadata" do
+
+    let(:repo_paths) do
+      [
+        File.join(CHEF_SPEC_DATA, "kitchen"),
+        File.join(CHEF_SPEC_DATA, "cookbooks"),
+        File.join(CHEF_SPEC_DATA, "invalid-metadata-chef-repo")
+      ]
+    end
+
+    it "does not squelch the exception" do
+      expect { cookbook_loader.load_cookbooks }.to raise_error("THIS METADATA HAS A BUG")
+    end
+
+  end
+
   describe "loading only one cookbook" do
     before(:each) do
       cookbook_loader.load_cookbook("openldap")
@@ -217,6 +233,25 @@ describe Chef::CookbookLoader do
       cookbook_loader["apache2"].should be_a_kind_of(Chef::CookbookVersion)
     end
 
+    context "when an unrelated cookbook has invalid metadata" do
+
+      let(:repo_paths) do
+        [
+          File.join(CHEF_SPEC_DATA, "kitchen"),
+          File.join(CHEF_SPEC_DATA, "cookbooks"),
+          File.join(CHEF_SPEC_DATA, "invalid-metadata-chef-repo")
+        ]
+      end
+
+      it "ignores the invalid cookbook" do
+        expect { cookbook_loader["openldap"] }.to_not raise_error
+      end
+
+      it "surfaces the exception if the cookbook is loaded later" do
+        expect { cookbook_loader["invalid-metadata"] }.to raise_error("THIS METADATA HAS A BUG")
+      end
+
+    end
 
     describe "loading all cookbooks after loading only one cookbook" do
       before(:each) do