Optionally check for cookbook dependencies when using knife cookbook upload

Signed-off-by: Jason Barnett <jason.w.barnett@gmail.com>

2 files changed, 132 insertions, 49 deletions

diff --git a/knife/lib/chef/knife/cookbook_upload.rb b/knife/lib/chef/knife/cookbook_upload.rb
index d9582a3ccc..357019212f 100644
--- a/knife/lib/chef/knife/cookbook_upload.rb
+++ b/knife/lib/chef/knife/cookbook_upload.rb
@@ -71,6 +71,11 @@ class Chef
         long: "--include-dependencies",
         description: "Also upload cookbook dependencies."
 
+      option :check_dependencies,
+        boolean: true, long: "--[no-]check-dependencies",
+        description: "Whether or not cookbook dependencies are verified before uploading cookbook(s) to #{ChefUtils::Dist::Server::PRODUCT}. You shouldn't disable this unless you really know what you're doing.",
+        default: true
+
       def run
         # Sanity check before we load anything from the server
         if ! config[:all] && @name_args.empty?
@@ -86,11 +91,6 @@ class Chef
         upload_failures = 0
         upload_ok = 0
 
-        # Get a list of cookbooks and their versions from the server
-        # to check for the existence of a cookbook's dependencies.
-        @server_side_cookbooks = Chef::CookbookVersion.list_all_versions
-        justify_width = @server_side_cookbooks.map(&:size).max.to_i + 2
-
         cookbooks = []
         cookbooks_to_upload.each do |cookbook_name, cookbook|
           raise Chef::Exceptions::MetadataNotFound.new(cookbook.root_paths[0], cookbook_name) unless cookbook.has_metadata_file?
@@ -120,7 +120,7 @@ class Chef
             if config[:all]
               if cookbooks_for_upload.any?
                 begin
-                  upload(cookbooks_for_upload, justify_width)
+                  upload(cookbooks_for_upload)
                 rescue Chef::Exceptions::CookbookFrozen
                   ui.warn("Not updating version constraints for some cookbooks in the environment as the cookbook is frozen.")
                   ui.error("Uploading of some of the cookbooks must be failed. Remove cookbook whose version is frozen from your cookbooks repo OR use --force option.")
@@ -133,7 +133,7 @@ class Chef
             else
               tmp_cl.each do |cookbook_name, cookbook|
 
-                upload([cookbook], justify_width)
+                upload([cookbook])
                 upload_ok += 1
               rescue Exceptions::CookbookNotFoundInRepo => e
                 upload_failures += 1
@@ -165,6 +165,27 @@ class Chef
         end
       end
 
+      def server_side_cookbooks
+        @server_side_cookbooks ||= Chef::CookbookVersion.list_all_versions
+      end
+
+      def justify_width
+        @justify_width ||= server_side_cookbooks.map(&:size).max.to_i + 2
+      end
+
+      #
+      # @param cookbook [Chef::CookbookVersion]
+      #
+      def left_justify_name(cookbook)
+        # We only want to lookup justify width value if we're already loading
+        # cookbooks to check dependencies exist in Chef Infra Server.
+        if config[:check_dependencies] == true
+          cookbook.name.to_s.ljust(justify_width + 10)
+        else
+          cookbook.name.to_s.ljust(24)
+        end
+      end
+
       def cookbooks_to_upload
         @cookbooks_to_upload ||=
           if config[:all]
@@ -220,11 +241,11 @@ class Chef
         end
       end
 
-      def upload(cookbooks, justify_width)
+      def upload(cookbooks)
         cookbooks.each do |cb|
-          ui.info("Uploading #{cb.name.to_s.ljust(justify_width + 10)} [#{cb.version}]")
+          ui.info("Uploading #{left_justify_name(cb)} [#{cb.version}]")
           check_for_broken_links!(cb)
-          check_for_dependencies!(cb)
+          check_for_dependencies!(cb) if config[:check_dependencies] == true
         end
         Chef::CookbookUploader.new(cookbooks, force: config[:force], concurrency: config[:concurrency]).upload_cookbooks
       rescue Chef::Exceptions::CookbookFrozen => e
@@ -265,12 +286,12 @@ class Chef
       end
 
       def check_server_side_cookbooks(cookbook_name, version)
-        if @server_side_cookbooks[cookbook_name].nil?
+        if server_side_cookbooks[cookbook_name].nil?
           false
         else
-          versions = @server_side_cookbooks[cookbook_name]["versions"].collect { |versions| versions["version"] }
+          versions = server_side_cookbooks[cookbook_name]["versions"].collect { |versions| versions["version"] }
           Log.debug "Versions of cookbook '#{cookbook_name}' returned by the server: #{versions.join(", ")}"
-          @server_side_cookbooks[cookbook_name]["versions"].each do |versions_hash|
+          server_side_cookbooks[cookbook_name]["versions"].each do |versions_hash|
             if Chef::VersionConstraint.new(version).include?(versions_hash["version"])
               Log.debug "Matched cookbook '#{cookbook_name}' with constraint '#{version}' to cookbook version '#{versions_hash["version"]}' on the server"
               return true
diff --git a/knife/spec/unit/knife/cookbook_upload_spec.rb b/knife/spec/unit/knife/cookbook_upload_spec.rb
index 0893f6a6b3..a61eee9249 100644
--- a/knife/spec/unit/knife/cookbook_upload_spec.rb
+++ b/knife/spec/unit/knife/cookbook_upload_spec.rb
@@ -60,11 +60,12 @@ describe Chef::Knife::CookbookUpload do
   before(:each) do
     allow(Chef::CookbookLoader).to receive(:new).and_return(cookbook_loader)
     allow(Chef::CookbookLoader).to receive(:copy_to_tmp_dir_from_array).and_yield(cookbook_loader)
+    allow(Chef::CookbookVersion).to receive(:list).and_return({})
+    allow(Chef::CookbookVersion).to receive(:list_all_versions).and_return({})
   end
 
   describe "with --concurrency" do
     it "should upload cookbooks with predefined concurrency" do
-      allow(Chef::CookbookVersion).to receive(:list_all_versions).and_return({})
       knife.config[:concurrency] = 3
       test_cookbook = Chef::CookbookVersion.new("test_cookbook", "/tmp/blah")
       allow(cookbook_loader).to receive(:each).and_yield("test_cookbook", test_cookbook)
@@ -79,7 +80,6 @@ describe Chef::Knife::CookbookUpload do
   describe "run" do
     before(:each) do
       allow(Chef::CookbookUploader).to receive_messages(new: cookbook_uploader)
-      allow(Chef::CookbookVersion).to receive(:list_all_versions).and_return({})
     end
 
     it "should print usage and exit when a cookbook name is not provided" do
@@ -214,48 +214,110 @@ describe Chef::Knife::CookbookUpload do
       end
     end
 
-    describe "when specifying a cookbook name with missing dependencies" do
-      let(:cookbook_dependency) { Chef::CookbookVersion.new("dependency", "/tmp/blah") }
+    context "when chef_dependencies config is disabled" do
+      before do
+        knife.config[:check_dependencies] = false
+      end
 
-      before(:each) do
-        cookbook.metadata.depends("dependency")
-        allow(cookbook_loader).to receive(:[]) do |ckbk|
-          { "test_cookbook" => cookbook,
-            "dependency" => cookbook_dependency }[ckbk]
+      describe "when specifying a cookbook name with missing dependencies" do
+        let(:cookbook_dependency) { Chef::CookbookVersion.new("dependency", "/tmp/blah") }
+
+        before(:each) do
+          cookbook.metadata.depends("dependency")
+          allow(cookbook_loader).to receive(:[]) do |ckbk|
+            { "test_cookbook" => cookbook,
+              "dependency" => cookbook_dependency }[ckbk]
+          end
+          allow(knife).to receive(:cookbook_names).and_return(%w{cookbook_dependency test_cookbook})
+          @stdout, @stderr, @stdin = StringIO.new, StringIO.new, StringIO.new
+          knife.ui = Chef::Knife::UI.new(@stdout, @stderr, @stdin, {})
         end
-        allow(knife).to receive(:cookbook_names).and_return(%w{cookbook_dependency test_cookbook})
-        @stdout, @stderr, @stdin = StringIO.new, StringIO.new, StringIO.new
-        knife.ui = Chef::Knife::UI.new(@stdout, @stderr, @stdin, {})
-      end
 
-      it "should exit and not upload the cookbook" do
-        expect(cookbook_loader).to receive(:[]).once.with("test_cookbook")
-        expect(cookbook_uploader).not_to receive(:upload_cookbooks)
-        expect { knife.run }.to raise_error(SystemExit)
+        it "should not fetch all cookbooks from Chef Infra Server" do
+          expect(Chef::CookbookVersion).not_to receive(:list_all_versions)
+          knife.run
+        end
+
+        it "should upload the cookbook" do
+          expect(cookbook_loader).to receive(:[]).once.with("test_cookbook")
+          expect(cookbook_uploader).to receive(:upload_cookbooks)
+          knife.run
+        end
+
+        it "should not output a message for a single missing dependency" do
+          knife.run
+          expect(@stderr.string).not_to include("Cookbook test_cookbook depends on cookbooks which are not currently")
+          expect(@stderr.string).not_to include("being uploaded and cannot be found on the server.")
+          expect(@stderr.string).not_to include("The missing cookbook(s) are: 'dependency' version '>= 0.0.0'")
+        end
+
+        it "should not output a message for a multiple missing dependencies which are concatenated" do
+          cookbook_dependency2 = Chef::CookbookVersion.new("dependency2")
+          cookbook.metadata.depends("dependency2")
+          allow(cookbook_loader).to receive(:[]) do |ckbk|
+            { "test_cookbook" => cookbook,
+              "dependency" => cookbook_dependency,
+              "dependency2" => cookbook_dependency2 }[ckbk]
+          end
+          allow(knife).to receive(:cookbook_names).and_return(%w{dependency dependency2 test_cookbook})
+          knife.run
+          expect(@stderr.string).not_to include("Cookbook test_cookbook depends on cookbooks which are not currently")
+          expect(@stderr.string).not_to include("being uploaded and cannot be found on the server.")
+          expect(@stderr.string).not_to include("The missing cookbook(s) are:")
+          expect(@stderr.string).not_to include("'dependency' version '>= 0.0.0'")
+          expect(@stderr.string).not_to include("'dependency2' version '>= 0.0.0'")
+        end
       end
+    end
 
-      it "should output a message for a single missing dependency" do
-        expect { knife.run }.to raise_error(SystemExit)
-        expect(@stderr.string).to include("Cookbook test_cookbook depends on cookbooks which are not currently")
-        expect(@stderr.string).to include("being uploaded and cannot be found on the server.")
-        expect(@stderr.string).to include("The missing cookbook(s) are: 'dependency' version '>= 0.0.0'")
+    context "when chef_dependencies config is enabled" do
+      before do
+        knife.config[:check_dependencies] = true
       end
 
-      it "should output a message for a multiple missing dependencies which are concatenated" do
-        cookbook_dependency2 = Chef::CookbookVersion.new("dependency2")
-        cookbook.metadata.depends("dependency2")
-        allow(cookbook_loader).to receive(:[]) do |ckbk|
-          { "test_cookbook" => cookbook,
-            "dependency" => cookbook_dependency,
-            "dependency2" => cookbook_dependency2 }[ckbk]
+      describe "when specifying a cookbook name with missing dependencies" do
+        let(:cookbook_dependency) { Chef::CookbookVersion.new("dependency", "/tmp/blah") }
+
+        before(:each) do
+          cookbook.metadata.depends("dependency")
+          allow(cookbook_loader).to receive(:[]) do |ckbk|
+            { "test_cookbook" => cookbook,
+              "dependency" => cookbook_dependency }[ckbk]
+          end
+          allow(knife).to receive(:cookbook_names).and_return(%w{cookbook_dependency test_cookbook})
+          @stdout, @stderr, @stdin = StringIO.new, StringIO.new, StringIO.new
+          knife.ui = Chef::Knife::UI.new(@stdout, @stderr, @stdin, {})
+        end
+
+        it "should exit and not upload the cookbook" do
+          expect(cookbook_loader).to receive(:[]).once.with("test_cookbook")
+          expect(cookbook_uploader).not_to receive(:upload_cookbooks)
+          expect { knife.run }.to raise_error(SystemExit)
+        end
+
+        it "should output a message for a single missing dependency" do
+          expect { knife.run }.to raise_error(SystemExit)
+          expect(@stderr.string).to include("Cookbook test_cookbook depends on cookbooks which are not currently")
+          expect(@stderr.string).to include("being uploaded and cannot be found on the server.")
+          expect(@stderr.string).to include("The missing cookbook(s) are: 'dependency' version '>= 0.0.0'")
+        end
+
+        it "should output a message for a multiple missing dependencies which are concatenated" do
+          cookbook_dependency2 = Chef::CookbookVersion.new("dependency2")
+          cookbook.metadata.depends("dependency2")
+          allow(cookbook_loader).to receive(:[]) do |ckbk|
+            { "test_cookbook" => cookbook,
+              "dependency" => cookbook_dependency,
+              "dependency2" => cookbook_dependency2 }[ckbk]
+          end
+          allow(knife).to receive(:cookbook_names).and_return(%w{dependency dependency2 test_cookbook})
+          expect { knife.run }.to raise_error(SystemExit)
+          expect(@stderr.string).to include("Cookbook test_cookbook depends on cookbooks which are not currently")
+          expect(@stderr.string).to include("being uploaded and cannot be found on the server.")
+          expect(@stderr.string).to include("The missing cookbook(s) are:")
+          expect(@stderr.string).to include("'dependency' version '>= 0.0.0'")
+          expect(@stderr.string).to include("'dependency2' version '>= 0.0.0'")
         end
-        allow(knife).to receive(:cookbook_names).and_return(%w{dependency dependency2 test_cookbook})
-        expect { knife.run }.to raise_error(SystemExit)
-        expect(@stderr.string).to include("Cookbook test_cookbook depends on cookbooks which are not currently")
-        expect(@stderr.string).to include("being uploaded and cannot be found on the server.")
-        expect(@stderr.string).to include("The missing cookbook(s) are:")
-        expect(@stderr.string).to include("'dependency' version '>= 0.0.0'")
-        expect(@stderr.string).to include("'dependency2' version '>= 0.0.0'")
       end
     end