Merge pull request #7792 from chef/lcg/chef-15-cookbook-loader

Remove cookbook merging/shadowing from the cookbooker loader

5 files changed, 27 insertions, 142 deletions

diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index 32fd71f604..322de539f1 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -44,7 +44,6 @@ class Chef
       @cookbooks_by_name = Mash.new
       @loaded_cookbooks = {}
       @metadata = Mash.new
-      @cookbooks_paths = Hash.new { |h, k| h[k] = [] } # for deprecation warnings
       @chefignores = {}
       @repo_paths = repo_paths.map do |repo_path|
         File.expand_path(repo_path)
@@ -52,31 +51,9 @@ class Chef
 
       @preloaded_cookbooks = false
       @loaders_by_name = {}
-
-      # Used to track which cookbooks appear in multiple places in the cookbook repos
-      # and are merged in to a single cookbook by file shadowing. This behavior is
-      # deprecated, so users of this class may issue warnings to the user by checking
-      # this variable
-      @merged_cookbooks = []
-    end
-
-    def merged_cookbook_paths # for deprecation warnings
-      merged_cookbook_paths = {}
-      @merged_cookbooks.each { |c| merged_cookbook_paths[c] = @cookbooks_paths[c] }
-      merged_cookbook_paths
     end
 
-    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
+    def load_cookbooks
       preload_cookbooks
       @loaders_by_name.each_key do |cookbook_name|
         load_cookbook(cookbook_name)
@@ -84,12 +61,6 @@ 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
 
@@ -102,14 +73,11 @@ class Chef
 
         next if loader.empty?
 
-        @cookbooks_paths[cookbook_name] << loader.cookbook_path # for deprecation warnings
-
         if @loaded_cookbooks.key?(cookbook_name)
-          @merged_cookbooks << cookbook_name # for deprecation warnings
-          @loaded_cookbooks[cookbook_name].merge!(loader)
-        else
-          @loaded_cookbooks[cookbook_name] = loader
+          raise Chef::Exceptions::CookbookMergingError, "Cookbook merging is no longer supported, the cookbook named #{cookbook_name} can only appear once in the cookbook_path"
         end
+
+        @loaded_cookbooks[cookbook_name] = loader
       end
 
       if @loaded_cookbooks.key?(cookbook_name)
@@ -133,6 +101,7 @@ class Chef
     def has_key?(cookbook_name)
       not self[cookbook_name.to_sym].nil?
     end
+
     alias :cookbook_exists? :has_key?
     alias :key? :has_key?
 
@@ -157,6 +126,7 @@ class Chef
     def values
       @cookbooks_by_name.values
     end
+
     alias :cookbooks :values
 
     private
diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index dafd445d2d..41e0cdb33e 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -2,7 +2,7 @@
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: Seth Falcon (<seth@chef.io>)
 # Author:: Kyle Goodwin (<kgoodwin@primerevenue.com>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -99,6 +99,7 @@ class Chef
     # Cookbook loader used to raise an argument error when cookbook not found.
     # for back compat, need to raise an error that inherits from ArgumentError
     class CookbookNotFoundInRepo < ArgumentError; end
+    class CookbookMergingError < RuntimeError; end
     class RecipeNotFound < ArgumentError; end
     # AttributeNotFound really means the attribute file could not be found
     class AttributeNotFound < RuntimeError; end
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index ba63e7d0fd..4260ee01e2 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -93,7 +93,6 @@ class Chef
         end
 
         assert_environment_valid!
-        warn_about_cookbook_shadowing
         version_constraints_to_update = {}
         upload_failures = 0
         upload_ok = 0
@@ -103,7 +102,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_without_shadow_warning
+          cookbook_repo.load_cookbooks
           cookbooks_for_upload = []
           cookbook_repo.each do |cookbook_name, cookbook|
             cookbooks_for_upload << cookbook
@@ -164,7 +163,7 @@ class Chef
       def cookbooks_to_upload
         @cookbooks_to_upload ||=
           if config[:all]
-            cookbook_repo.load_cookbooks_without_shadow_warning
+            cookbook_repo.load_cookbooks
           else
             upload_set = {}
             @name_args.each do |cookbook_name|
@@ -202,26 +201,6 @@ class Chef
         @environment ||= config[:environment] ? Environment.load(config[:environment]) : nil
       end
 
-      def warn_about_cookbook_shadowing
-        # because cookbooks are lazy-loaded, we have to force the loader
-        # to load the cookbooks the user intends to upload here:
-        cookbooks_to_upload
-
-        unless cookbook_repo.merged_cookbooks.empty?
-          ui.warn "* " * 40
-          ui.warn(<<~WARNING)
-            The cookbooks: #{cookbook_repo.merged_cookbooks.join(', ')} exist in multiple places in your cookbook_path.
-            A composite version of these cookbooks has been compiled for uploading.
-
-            #{ui.color('IMPORTANT:', :red, :bold)} In a future version of Chef, this behavior will be removed and you will no longer
-            be able to have the same version of a cookbook in multiple places in your cookbook_path.
-WARNING
-          ui.warn "The affected cookbooks are located:"
-          ui.output ui.format_for_display(cookbook_repo.merged_cookbook_paths)
-          ui.warn "* " * 40
-        end
-      end
-
       private
 
       def assert_environment_valid!
diff --git a/spec/unit/cookbook_loader_spec.rb b/spec/unit/cookbook_loader_spec.rb
index 6553dad433..15c7d1e299 100644
--- a/spec/unit/cookbook_loader_spec.rb
+++ b/spec/unit/cookbook_loader_spec.rb
@@ -24,7 +24,6 @@ describe Chef::CookbookLoader do
   end
   let(:repo_paths) do
     [
-      File.expand_path(File.join(CHEF_SPEC_DATA, "kitchen")),
       File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks")),
     ]
   end
@@ -49,26 +48,25 @@ 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
+  context "removed cookbook merging" do
+    let(:repo_paths) do
+      [
+        File.expand_path(File.join(CHEF_SPEC_DATA, "kitchen")),
+        File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks")),
+      ]
     end
-
-    it "should be possible to reload all the cookbooks without triggering deprecation warnings on all of them", chef: "< 15" do
+    it "should not support multiple merged cookbooks in the cookbook path" 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)
+      expect { cookbook_loader.load_cookbooks }.to raise_error(Chef::Exceptions::CookbookMergingError)
     end
+  end
 
-    it "should not support multiple merged cookbooks in the cookbook path", chef: ">= 15" do
-      start_merged_cookbooks = cookbook_loader.merged_cookbooks
-      expect { cookbook_loader.load_cookbooks }.to raise_error("FIXME WITH THE CLASS YOU DECIDE TO USE HERE")
+  context "after loading all cookbooks" do
+    before(:each) do
+      cookbook_loader.load_cookbooks
     end
 
     describe "[]" do
@@ -114,57 +112,24 @@ describe Chef::CookbookLoader do
         expect(cookbook_loader).to have_key(:apache2)
       end
 
-      it "should allow you to override an attribute file via cookbook_path" do
-        expect(full_paths_for_part(:openldap, "attributes").detect do |f|
-          f =~ /cookbooks\/openldap\/attributes\/default.rb/
-        end).not_to eql(nil)
-        expect(full_paths_for_part(:openldap, "attributes").detect do |f|
-          f =~ /kitchen\/openldap\/attributes\/default.rb/
-        end).to eql(nil)
-      end
-
       it "should load different attribute files from deeper paths" do
         expect(full_paths_for_part(:openldap, "attributes").detect do |f|
-          f =~ /kitchen\/openldap\/attributes\/robinson.rb/
-        end).not_to eql(nil)
-      end
-
-      it "should allow you to override a definition file via cookbook_path" do
-        expect(full_paths_for_part(:openldap, "definitions").detect do |f|
-          f =~ /cookbooks\/openldap\/definitions\/client.rb/
+          f =~ /cookbooks\/openldap\/attributes\/smokey.rb/
         end).not_to eql(nil)
-        expect(full_paths_for_part(:openldap, "definitions").detect do |f|
-          f =~ /kitchen\/openldap\/definitions\/client.rb/
-        end).to eql(nil)
       end
 
       it "should load definition files from deeper paths" do
         expect(full_paths_for_part(:openldap, "definitions").detect do |f|
-          f =~ /kitchen\/openldap\/definitions\/drewbarrymore.rb/
+          f =~ /cookbooks\/openldap\/definitions\/server.rb/
         end).not_to eql(nil)
       end
 
-      it "should allow you to override a recipe file via cookbook_path" do
-        expect(full_paths_for_part(:openldap, "recipes").detect do |f|
-          f =~ /cookbooks\/openldap\/recipes\/gigantor.rb/
-        end).not_to eql(nil)
-        expect(full_paths_for_part(:openldap, "recipes").detect do |f|
-          f =~ /kitchen\/openldap\/recipes\/gigantor.rb/
-        end).to eql(nil)
-      end
-
       it "should load recipe files from deeper paths" do
         expect(full_paths_for_part(:openldap, "recipes").detect do |f|
-          f =~ /kitchen\/openldap\/recipes\/woot.rb/
+          f =~ /cookbooks\/openldap\/recipes\/one.rb/
         end).not_to eql(nil)
       end
 
-      it "should allow you to have an 'ignore' file, which skips loading files in later cookbooks" do
-        expect(full_paths_for_part(:openldap, "recipes").detect do |f|
-          f =~ /kitchen\/openldap\/recipes\/ignoreme.rb/
-        end).to eql(nil)
-      end
-
       it "should find files that start with a ." do
         expect(full_paths_for_part(:openldap, "files").detect do |f|
           f =~ /\.dotfile$/
@@ -187,7 +152,6 @@ describe Chef::CookbookLoader 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"),
       ]
@@ -253,7 +217,6 @@ describe Chef::CookbookLoader 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"),
         ]
@@ -271,7 +234,6 @@ 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 74612f520d..ba0d57c5d6 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -1,7 +1,7 @@
 #
 # Author:: Matthew Kent (<mkent@magoazul.com>)
 # Author:: Steven Danna (<steve@chef.io>)
-# Copyright:: Copyright 2012-2016, Chef Software Inc.
+# Copyright:: Copyright 2012-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -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_without_shadow_warning).and_return(cookbook_loader)
+    allow(cookbook_loader).to receive(:load_cookbooks).and_return(cookbook_loader)
     cookbook_loader
   end
 
@@ -102,31 +102,6 @@ describe Chef::Knife::CookbookUpload do
       end
     end
 
-    context "when uploading a cookbook that uses deprecated overlays" do
-
-      before do
-        allow(cookbook_loader).to receive(:merged_cookbooks).and_return(["test_cookbook"])
-        allow(cookbook_loader).to receive(:merged_cookbook_paths)
-          .and_return({ "test_cookbook" => %w{/path/one/test_cookbook /path/two/test_cookbook} })
-      end
-
-      it "emits a warning" do
-        knife.run
-        expected_message = <<~E
-          WARNING: The cookbooks: test_cookbook exist in multiple places in your cookbook_path.
-          A composite version of these cookbooks has been compiled for uploading.
-
-          IMPORTANT: In a future version of Chef, this behavior will be removed and you will no longer
-          be able to have the same version of a cookbook in multiple places in your cookbook_path.
-          WARNING: The affected cookbooks are located:
-          test_cookbook:
-            /path/one/test_cookbook
-            /path/two/test_cookbook
-E
-        expect(output.string).to include(expected_message)
-      end
-    end
-
     describe "when specifying a cookbook name among many" do
       let(:name_args) { ["test_cookbook1"] }
 
@@ -145,7 +120,6 @@ 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
 
@@ -209,7 +183,6 @@ 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