Merge pull request #4574 from chef/lcg/cookbook-shadow-deprecation

Extend cookbook shadowing deprecation warnings more broadly

4 files changed, 34 insertions, 5 deletions

diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index 9bab6f01ce..9367936c32 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -66,7 +66,17 @@ class Chef
       merged_cookbook_paths
     end
 
-    def load_cookbooks
+    def warn_about_cookbook_shadowing
+      unless merged_cookbooks.empty?
+        Chef::Log.deprecation "The cookbook(s): #{merged_cookbooks.join(', ')} exist in multiple places in your cookbook_path. " +
+          "A composite version has been compiled.  This has been deprecated since 0.10.4, in Chef 13 this behavior will be REMOVED."
+      end
+    end
+
+    # Will be removed when cookbook shadowing is removed, do NOT create new consumers of this API.
+    #
+    # @api private
+    def load_cookbooks_without_shadow_warning
       preload_cookbooks
       @loaders_by_name.each do |cookbook_name, _loaders|
         load_cookbook(cookbook_name)
@@ -74,9 +84,17 @@ class Chef
       @cookbooks_by_name
     end
 
+    def load_cookbooks
+      ret = load_cookbooks_without_shadow_warning
+      warn_about_cookbook_shadowing
+      ret
+    end
+
     def load_cookbook(cookbook_name)
       preload_cookbooks
 
+      return @cookbooks_by_name[cookbook_name] if @cookbooks_by_name.has_key?(cookbook_name)
+
       return nil unless @loaders_by_name.key?(cookbook_name.to_s)
 
       cookbook_loaders_for(cookbook_name).each do |loader|
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index cd16358e3f..72f7d9d2d9 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -103,7 +103,7 @@ class Chef
         @server_side_cookbooks = Chef::CookbookVersion.list_all_versions
         justify_width = @server_side_cookbooks.map { |name| name.size }.max.to_i + 2
         if config[:all]
-          cookbook_repo.load_cookbooks
+          cookbook_repo.load_cookbooks_without_shadow_warning
           cookbooks_for_upload = []
           cookbook_repo.each do |cookbook_name, cookbook|
             cookbooks_for_upload << cookbook
@@ -164,7 +164,7 @@ class Chef
       def cookbooks_to_upload
         @cookbooks_to_upload ||=
           if config[:all]
-            cookbook_repo.load_cookbooks
+            cookbook_repo.load_cookbooks_without_shadow_warning
           else
             upload_set = {}
             @name_args.each do |cookbook_name|
diff --git a/spec/unit/cookbook_loader_spec.rb b/spec/unit/cookbook_loader_spec.rb
index 3f4a7486ce..db10a4eb4f 100644
--- a/spec/unit/cookbook_loader_spec.rb
+++ b/spec/unit/cookbook_loader_spec.rb
@@ -45,14 +45,23 @@ describe Chef::CookbookLoader do
         once.
         and_call_original
     end
+    expect(Chef::Log).to receive(:deprecation).with(/The cookbook\(s\): openldap exist in multiple places in your cookbook_path./)
     cookbook_loader.load_cookbooks
   end
 
   context "after loading all cookbooks" do
     before(:each) do
+      expect(Chef::Log).to receive(:deprecation).with(/The cookbook\(s\): openldap exist in multiple places in your cookbook_path./)
       cookbook_loader.load_cookbooks
     end
 
+    it "should be possible to reload all the cookbooks without triggering deprecation warnings on all of them" do
+      start_merged_cookbooks = cookbook_loader.merged_cookbooks
+      expect(Chef::Log).to receive(:deprecation).with(/The cookbook\(s\): openldap exist in multiple places in your cookbook_path./)
+      cookbook_loader.load_cookbooks
+      expect(cookbook_loader.merged_cookbooks).to eql(start_merged_cookbooks)
+    end
+
     describe "[]" do
       it "should return cookbook objects with []" do
         expect(cookbook_loader[:openldap]).to be_a_kind_of(Chef::CookbookVersion)
@@ -98,7 +107,6 @@ describe Chef::CookbookLoader do
 
     describe "referencing cookbook files" do
       it "should find all the cookbooks in the cookbook path" do
-        cookbook_loader.load_cookbooks
         expect(cookbook_loader).to have_key(:openldap)
         expect(cookbook_loader).to have_key(:apache2)
       end
@@ -260,6 +268,7 @@ describe Chef::CookbookLoader do
 
     describe "loading all cookbooks after loading only one cookbook" do
       before(:each) do
+        expect(Chef::Log).to receive(:deprecation).with(/The cookbook\(s\): openldap exist in multiple places in your cookbook_path./)
         cookbook_loader.load_cookbooks
       end
 
diff --git a/spec/unit/knife/cookbook_upload_spec.rb b/spec/unit/knife/cookbook_upload_spec.rb
index 277da14011..9e07497c57 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -32,7 +32,7 @@ describe Chef::Knife::CookbookUpload do
   let(:cookbook_loader) do
     cookbook_loader = cookbooks_by_name.dup
     allow(cookbook_loader).to receive(:merged_cookbooks).and_return([])
-    allow(cookbook_loader).to receive(:load_cookbooks).and_return(cookbook_loader)
+    allow(cookbook_loader).to receive(:load_cookbooks_without_shadow_warning).and_return(cookbook_loader)
     cookbook_loader
   end
 
@@ -145,6 +145,7 @@ E
 
       it "should not read all cookbooks" do
         expect(cookbook_loader).not_to receive(:load_cookbooks)
+        expect(cookbook_loader).not_to receive(:load_cookbooks_without_shadow_warning)
         knife.run
       end
 
@@ -208,6 +209,7 @@ E
       it "should exit and not upload the cookbook" do
         expect(cookbook_loader).to receive(:[]).once.with("test_cookbook")
         expect(cookbook_loader).not_to receive(:load_cookbooks)
+        expect(cookbook_loader).not_to receive(:load_cookbooks_without_shadow_warning)
         expect(cookbook_uploader).not_to receive(:upload_cookbooks)
         expect { knife.run }.to raise_error(SystemExit)
       end