Merge pull request #10185 from chef/fix-cookbook-uploader-global-state

Remove cookbook uploader global state

4 files changed, 18 insertions, 43 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 63be9c9f4b..e9f973ef6f 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
@@ -76,8 +76,7 @@ class Chef
             raise Chef::Exceptions::MetadataNotFound.new(cookbook.root_paths[0], cookbook.name) unless cookbook.has_metadata_file?
 
             if cookbook
-              begin
-                tmp_cl = Chef::CookbookLoader.copy_to_tmp_dir_from_array([cookbook])
+              Chef::CookbookLoader.copy_to_tmp_dir_from_array([cookbook]) do |tmp_cl|
                 tmp_cl.load_cookbooks
                 tmp_cl.compile_metadata
                 tmp_cl.freeze_versions if options[:freeze]
@@ -91,9 +90,6 @@ class Chef
                 with_actual_cookbooks_dir(other.parent.file_path) do
                   uploader.upload_cookbooks
                 end
-
-              ensure
-                tmp_cl.unlink!
               end
             end
           end
diff --git a/lib/chef/cookbook_loader.rb b/lib/chef/cookbook_loader.rb
index e6e6a4805d..f7efb2646e 100644
--- a/lib/chef/cookbook_loader.rb
+++ b/lib/chef/cookbook_loader.rb
@@ -44,14 +44,11 @@ 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
@@ -140,27 +137,23 @@ class Chef
 
     # This method creates tmp directory and copies all cookbooks into it and creates cookbook loader 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)
+      Dir.mktmpdir do |tmp_dir|
+        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_dir, 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_dir)
+          CookbookLoader.new(tmp_dir)
+        end
+        yield tmp_cookbook_loader
       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
@@ -181,13 +174,6 @@ class Chef
       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 5a46972b43..b87aea7e04 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -107,8 +107,7 @@ class Chef
           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: '#{File.expand_path(cookbook_path)}'. Use --cookbook-path to specify the desired path.")
         else
-          begin
-            tmp_cl = Chef::CookbookLoader.copy_to_tmp_dir_from_array(cookbooks)
+          Chef::CookbookLoader.copy_to_tmp_dir_from_array(cookbooks) do |tmp_cl|
             tmp_cl.load_cookbooks
             tmp_cl.compile_metadata
             tmp_cl.freeze_versions if config[:freeze]
@@ -127,7 +126,6 @@ class Chef
                   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
                 rescue SystemExit => e
-                  tmp_cl.unlink!
                   raise exit e.status
                 end
                 ui.info("Uploaded all cookbooks.") if upload_failures == 0
@@ -146,7 +144,6 @@ class Chef
                   ui.warn("Not updating version constraints for #{cookbook_name} in the environment as the cookbook is frozen.")
                   upload_failures += 1
                 rescue SystemExit => e
-                  tmp_cl.unlink!
                   raise exit e.status
                 end
               end
@@ -164,8 +161,6 @@ class Chef
             unless version_constraints_to_update.empty?
               update_version_constraints(version_constraints_to_update) if config[:environment]
             end
-          ensure
-            tmp_cl.unlink!
           end
         end
       end
diff --git a/spec/unit/knife/cookbook_upload_spec.rb b/spec/unit/knife/cookbook_upload_spec.rb
index c667742187..dbed8b8a67 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -40,7 +40,6 @@ describe Chef::Knife::CookbookUpload do
     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
 
@@ -60,7 +59,7 @@ 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)
+    allow(Chef::CookbookLoader).to receive(:copy_to_tmp_dir_from_array).and_yield(cookbook_loader)
   end
 
   describe "with --concurrency" do
@@ -70,7 +69,6 @@ describe Chef::Knife::CookbookUpload do
       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))