Updated knife cookbook upload and knife upload code to create temporary directory and copies all cookbooks into it and then generate metadata.json if it does not exist and load it to upload on Chef server

Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

4 files changed, 110 insertions, 96 deletions

diff --git a/lib/chef/chef_fs/file_system/chef_server/cookbooks_dir.rb b/lib/chef/chef_fs/file_system/chef_server/cookbooks_dir.rb
index 09b6e0669f..b64eb2849e 100644
--- a/lib/chef/chef_fs/file_system/chef_server/cookbooks_dir.rb
+++ b/lib/chef/chef_fs/file_system/chef_server/cookbooks_dir.rb
@@ -72,35 +72,23 @@ class Chef
           end
 
           def upload_cookbook(other, options)
-            compiled_metadata = other.chef_object.compile_metadata
-
-            Dir.mktmpdir do |tmp_dir_path|
-              begin
-                if compiled_metadata
-                  proxy_cookbook_path = "#{tmp_dir_path}/#{other.name}"
-                  # Make a symlink
-                  file_class.symlink other.chef_object.root_dir, proxy_cookbook_path
-                  proxy_loader = Chef::Cookbook::CookbookVersionLoader.new(proxy_cookbook_path)
-                  proxy_loader.load_cookbooks
-                  cookbook_to_upload = proxy_loader.cookbook_version
-                else
-                  cookbook_to_upload = other.chef_object
-                end
-
-                cookbook_to_upload.freeze_version if options[:freeze]
-                uploader = Chef::CookbookUploader.new(cookbook_to_upload, force: options[:force], rest: chef_rest)
-
-                with_actual_cookbooks_dir(other.parent.file_path) do
-                  uploader.upload_cookbooks
-                end
-              ensure
-                # deletes the generated metadata from local repo.
-                if compiled_metadata
-                  GC.start
-                  File.unlink(compiled_metadata)
-                end
-              end
+            cookbook = other.chef_object
+            tmp_cl = Chef::CookbookLoader.copy_to_tmp_dir_from_array([cookbook])
+            tmp_cl.load_cookbooks
+            tmp_cl.compile_metadata
+            tmp_cl.freeze_version if options[:freeze]
+            cookbook_for_upload = []
+            tmp_cl.each do |cookbook_name, cookbook|
+              cookbook_for_upload << cookbook
             end
+
+            uploader = Chef::CookbookUploader.new(cookbook_for_upload, force: options[:force], rest: chef_rest)
+
+            with_actual_cookbooks_dir(other.parent.file_path) do
+              uploader.upload_cookbooks
+            end
+
+            tmp_cl.unlink!
           end
 
           def chef_rest
diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index 2bafed410d..bdb436e899 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -44,11 +44,14 @@ class Chef
     # @return [Array<String>] the array of repo paths containing cookbook dirs
     attr_reader :repo_paths
 
+    attr_accessor :tmp_working_dir_path
+
     # XXX: this is highly questionable combined with the Hash-style each method
     include Enumerable
 
     # @param repo_paths [Array<String>] the array of repo paths containing cookbook dirs
     def initialize(*repo_paths)
+      @tmp_working_dir_path = nil
       @repo_paths = repo_paths.flatten.compact.map { |p| File.expand_path(p) }
       raise ArgumentError, "You must specify at least one cookbook repo path" if @repo_paths.empty?
     end
@@ -135,6 +138,56 @@ class Chef
       cookbooks_by_name.values
     end
 
+    # This method creates tmp directory and copies all cookbooks into it and creates cookbook loder object which points to tmp directory
+    def self.copy_to_tmp_dir_from_array(cookbooks)
+      tmp_cookbook_file = Tempfile.new("tmp_working_dir_path")
+      tmp_cookbook_file.close
+      @tmp_working_dir_path = tmp_cookbook_file.path
+      File.unlink(@tmp_working_dir_path)
+      FileUtils.mkdir_p(@tmp_working_dir_path)
+      cookbooks.each do |cookbook|
+        checksums_to_on_disk_paths = cookbook.checksums
+        cookbook.each_file do |manifest_record|
+          path_in_cookbook = manifest_record[:path]
+          on_disk_path = checksums_to_on_disk_paths[manifest_record[:checksum]]
+          dest = File.join(@tmp_working_dir_path, cookbook.name.to_s, path_in_cookbook)
+          FileUtils.mkdir_p(File.dirname(dest))
+          FileUtils.cp_r(on_disk_path, dest)
+        end
+      end
+      tmp_cookbook_loader ||= begin
+        Chef::Cookbook::FileVendor.fetch_from_disk(@tmp_working_dir_path)
+        CookbookLoader.new(@tmp_working_dir_path)
+      end
+      tmp_cookbook_loader.tmp_working_dir_path = @tmp_working_dir_path
+      tmp_cookbook_loader
+    end
+
+    # generates metadata.json adds it in the manifest
+    def compile_metadata
+      each do |cookbook_name, cookbook|
+        compiled_metadata = cookbook.compile_metadata
+        if compiled_metadata
+          cookbook.all_files << compiled_metadata
+          cookbook.cookbook_manifest.send(:generate_manifest)
+        end
+      end
+    end
+
+    # freeze versions of all the cookbooks
+    def freeze_versions
+      each do |cookbook_name, cookbook|
+        cookbook.freeze_version
+      end
+    end
+
+    # removes the tmp_dir_path
+    def unlink!
+      raise "Invalid directory path." if @tmp_working_dir_path.nil?
+
+      FileUtils.rm_rf(@tmp_working_dir_path)
+    end
+
     alias :cookbooks :values
 
     private
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index aa6be3f1a1..a8b2635081 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -96,32 +96,42 @@ class Chef
         # 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|
+          cookbooks << cookbook
+        end
+
+        tmp_cl = Chef::CookbookLoader.copy_to_tmp_dir_from_array(cookbooks)
+        tmp_cl.load_cookbooks
+        tmp_cl.compile_metadata
+        tmp_cl.freeze_versions if config[:freeze]
+
+        cookbooks_for_upload = []
+        tmp_cl.each do |cookbook_name, cookbook|
+          cookbooks_for_upload << cookbook
+          version_constraints_to_update[cookbook_name] = cookbook.version
+        end
+
         if config[:all]
-          cookbook_repo.load_cookbooks
-          cookbooks_for_upload = []
-          cookbook_repo.each do |cookbook_name, cookbook|
-            cookbooks_for_upload << cookbook
-            cookbook.freeze_version if config[:freeze]
-            version_constraints_to_update[cookbook_name] = cookbook.version
-          end
           if cookbooks_for_upload.any?
             begin
               upload(cookbooks_for_upload, justify_width)
-            rescue Exceptions::CookbookFrozen
+            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.")
+              upload_failures += 1
             end
-            ui.info("Uploaded all cookbooks.")
+            ui.info("Uploaded all cookbooks.") if upload_failures == 0
           else
             cookbook_path = config[:cookbook_path].respond_to?(:join) ? config[:cookbook_path].join(", ") : config[:cookbook_path]
             ui.warn("Could not find any cookbooks in your cookbook path: #{cookbook_path}. Use --cookbook-path to specify the desired path.")
           end
         else
-          cookbooks_to_upload.each do |cookbook_name, cookbook|
-            cookbook.freeze_version if config[:freeze]
+          tmp_cl.each do |cookbook_name, cookbook|
             begin
               upload([cookbook], justify_width)
               upload_ok += 1
-              version_constraints_to_update[cookbook_name] = cookbook.version
             rescue Exceptions::CookbookNotFoundInRepo => e
               upload_failures += 1
               ui.error("Could not find cookbook #{cookbook_name} in your cookbook path, skipping it")
@@ -147,6 +157,8 @@ class Chef
         unless version_constraints_to_update.empty?
           update_version_constraints(version_constraints_to_update) if config[:environment]
         end
+
+        tmp_cl.unlink!
       end
 
       def cookbooks_to_upload
@@ -205,46 +217,15 @@ class Chef
       end
 
       def upload(cookbooks, justify_width)
-        require "tmpdir"
-
-        Dir.mktmpdir do |tmp_dir_path|
-          begin
-            cookbooks_to_upload = []
-            compiled_metadata = nil
-
-            cookbooks.each do |cb|
-              compiled_metadata = cb.compile_metadata
-              ui.info("Uploading #{cb.name.to_s.ljust(justify_width + 10)} [#{cb.version}]")
-              check_for_broken_links!(cb)
-              check_for_dependencies!(cb)
-
-              if compiled_metadata
-                proxy_cookbook_path = "#{tmp_dir_path}/#{cb.name}"
-                # Make a symlink
-                file_class.symlink cb.root_dir, proxy_cookbook_path
-                proxy_loader = Chef::Cookbook::CookbookVersionLoader.new(proxy_cookbook_path)
-
-                proxy_loader.load_cookbooks
-                cookbook_to_upload = proxy_loader.cookbook_version
-                cookbook_to_upload.freeze_version if options[:freeze]
-                cookbooks_to_upload << cookbook_to_upload
-              else
-                cookbooks_to_upload << cb
-              end
-            end
-
-            Chef::CookbookUploader.new(cookbooks_to_upload, force: config[:force], concurrency: config[:concurrency]).upload_cookbooks
-          rescue Chef::Exceptions::CookbookFrozen => e
-            ui.error e
-            raise
-          ensure
-            # deletes the generated metadata from local repo.
-            if compiled_metadata
-              GC.start
-              File.unlink(compiled_metadata)
-            end
-          end
+        cookbooks.each do |cb|
+          ui.info("Uploading #{cb.name.to_s.ljust(justify_width + 10)} [#{cb.version}]")
+          check_for_broken_links!(cb)
+          check_for_dependencies!(cb)
         end
+        Chef::CookbookUploader.new(cookbooks, force: config[:force], concurrency: config[:concurrency]).upload_cookbooks
+      rescue Chef::Exceptions::CookbookFrozen => e
+        ui.error e
+        raise
       end
 
       def check_for_broken_links!(cookbook)
diff --git a/spec/unit/knife/cookbook_upload_spec.rb b/spec/unit/knife/cookbook_upload_spec.rb
index 84b16ae215..ffea69a860 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -33,6 +33,9 @@ describe Chef::Knife::CookbookUpload 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(:compile_metadata).and_return(nil)
+    allow(cookbook_loader).to receive(:freeze_versions).and_return(nil)
+    allow(cookbook_loader).to receive(:unlink!).and_return(nil)
     cookbook_loader
   end
 
@@ -52,16 +55,17 @@ 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_return(cookbook_loader)
   end
 
   describe "with --concurrency" do
     it "should upload cookbooks with predefined concurrency" do
-      allow(cookbook).to receive(:compile_metadata).and_return(nil)
       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)
       allow(cookbook_loader).to receive(:cookbook_names).and_return(["test_cookbook"])
+      allow(cookbook_loader).to receive(:tmp_working_dir_path).and_return("/tmp/blah")
       expect(Chef::CookbookUploader).to receive(:new)
         .with( kind_of(Array), { force: nil, concurrency: 3 })
         .and_return(double("Chef::CookbookUploader", upload_cookbooks: true))
@@ -108,21 +112,17 @@ describe Chef::Knife::CookbookUpload do
 
       let(:cookbooks_by_name) do
         {
-          "test_cookbook1" => Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah"),
-          "test_cookbook2" => Chef::CookbookVersion.new("test_cookbook2", "/tmp/blah"),
-          "test_cookbook3" => Chef::CookbookVersion.new("test_cookbook3", "/tmp/blah"),
+          "test_cookbook1" => Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
         }
       end
 
       it "should read only one cookbook" do
-        allow(cookbooks_by_name["test_cookbook1"]).to receive(:compile_metadata).and_return(nil)
         expect(cookbook_loader).to receive(:[]).once.with("test_cookbook1").and_call_original
         knife.run
       end
 
       it "should not read all cookbooks" do
-        allow(cookbooks_by_name["test_cookbook1"]).to receive(:compile_metadata).and_return(nil)
-        expect(cookbook_loader).not_to receive(:load_cookbooks)
+        expect(cookbook_loader).to receive(:load_cookbooks)
         knife.run
       end
 
@@ -181,12 +181,11 @@ describe Chef::Knife::CookbookUpload do
         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, {})
-        allow(cookbook).to receive(:compile_metadata).and_return(nil)
       end
 
       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)
         expect(cookbook_uploader).not_to receive(:upload_cookbooks)
         expect { knife.run }.to raise_error(SystemExit)
       end
@@ -217,9 +216,8 @@ describe Chef::Knife::CookbookUpload do
     end
 
     it "should freeze the version of the cookbooks if --freeze is specified" do
-      allow(cookbook).to receive(:compile_metadata).and_return(nil)
       knife.config[:freeze] = true
-      expect(cookbook).to receive(:freeze_version).once
+      expect(cookbook_loader).to receive(:freeze_versions).once
       knife.run
     end
 
@@ -234,8 +232,6 @@ describe Chef::Knife::CookbookUpload do
           @test_cookbook2 = Chef::CookbookVersion.new("test_cookbook2", "/tmp/blah")
           allow(cookbook_loader).to receive(:each).and_yield("test_cookbook1", @test_cookbook1).and_yield("test_cookbook2", @test_cookbook2)
           allow(cookbook_loader).to receive(:cookbook_names).and_return(%w{test_cookbook1 test_cookbook2})
-          allow(@test_cookbook1).to receive(:compile_metadata).and_return(nil)
-          allow(@test_cookbook2).to receive(:compile_metadata).and_return(nil)
         end
 
         it "should upload all cookbooks" do
@@ -290,10 +286,6 @@ describe Chef::Knife::CookbookUpload do
     end
 
     describe "when a frozen cookbook exists on the server" do
-      before(:each) do
-        allow(cookbook).to receive(:compile_metadata).and_return(nil)
-      end
-
       it "should fail to replace it" do
         exception = Chef::Exceptions::CookbookFrozen.new
         expect(cookbook_uploader).to receive(:upload_cookbooks)