Updated code to make sure temporary directory gets deleted after the upload

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

3 files changed, 83 insertions, 68 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 5db69b69bb..3c13bca3a3 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,23 +72,29 @@ class Chef
           end
 
           def upload_cookbook(other, options)
-            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_versions if options[:freeze]
-            cookbook_for_upload = []
-            tmp_cl.each do |cookbook_name, cookbook|
-              cookbook_for_upload << cookbook
+            cookbook = other.chef_object if other.chef_object
+
+            if cookbook
+              begin
+                tmp_cl = Chef::CookbookLoader.copy_to_tmp_dir_from_array([cookbook])
+                tmp_cl.load_cookbooks
+                tmp_cl.compile_metadata
+                tmp_cl.freeze_versions 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
+
+              ensure
+                tmp_cl.unlink!
+              end
             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/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index a8b2635081..0243b0cf12 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -101,64 +101,74 @@ class Chef
         cookbooks_to_upload.each do |cookbook_name, cookbook|
           cookbooks << cookbook
         end
+        if cookbooks.empty?
+          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.")
+        else
+          begin
+            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
 
-        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]
+              if cookbooks_for_upload.any?
+                begin
+                  upload(cookbooks_for_upload, justify_width)
+                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
+                rescue SystemExit => e
+                  tmp_cl.unlink!
+                  raise exit e.status
+                end
+                ui.info("Uploaded all cookbooks.") if upload_failures == 0
+              end
+            else
+              tmp_cl.each do |cookbook_name, cookbook|
+                begin
+                  upload([cookbook], justify_width)
+                  upload_ok += 1
+                rescue Exceptions::CookbookNotFoundInRepo => e
+                  upload_failures += 1
+                  ui.error("Could not find cookbook #{cookbook_name} in your cookbook path, skipping it")
+                  Log.debug(e)
+                  upload_failures += 1
+                rescue Exceptions::CookbookFrozen
+                  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
 
-        if config[:all]
-          if cookbooks_for_upload.any?
-            begin
-              upload(cookbooks_for_upload, justify_width)
-            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
+              if upload_failures == 0
+                ui.info "Uploaded #{upload_ok} cookbook#{upload_ok == 1 ? "" : "s"}."
+              elsif upload_failures > 0 && upload_ok > 0
+                ui.warn "Uploaded #{upload_ok} cookbook#{upload_ok == 1 ? "" : "s"} ok but #{upload_failures} " +
+                  "cookbook#{upload_failures == 1 ? "" : "s"} upload failed."
+              elsif upload_failures > 0 && upload_ok == 0
+                ui.error "Failed to upload #{upload_failures} cookbook#{upload_failures == 1 ? "" : "s"}."
+                exit 1
+              end
             end
-            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
-          tmp_cl.each do |cookbook_name, cookbook|
-            begin
-              upload([cookbook], justify_width)
-              upload_ok += 1
-            rescue Exceptions::CookbookNotFoundInRepo => e
-              upload_failures += 1
-              ui.error("Could not find cookbook #{cookbook_name} in your cookbook path, skipping it")
-              Log.debug(e)
-              upload_failures += 1
-            rescue Exceptions::CookbookFrozen
-              ui.warn("Not updating version constraints for #{cookbook_name} in the environment as the cookbook is frozen.")
-              upload_failures += 1
+
+            unless version_constraints_to_update.empty?
+              update_version_constraints(version_constraints_to_update) if config[:environment]
             end
-          end
 
-          if upload_failures == 0
-            ui.info "Uploaded #{upload_ok} cookbook#{upload_ok == 1 ? "" : "s"}."
-          elsif upload_failures > 0 && upload_ok > 0
-            ui.warn "Uploaded #{upload_ok} cookbook#{upload_ok == 1 ? "" : "s"} ok but #{upload_failures} " +
-              "cookbook#{upload_failures == 1 ? "" : "s"} upload failed."
-          elsif upload_failures > 0 && upload_ok == 0
-            ui.error "Failed to upload #{upload_failures} cookbook#{upload_failures == 1 ? "" : "s"}."
-            exit 1
+          ensure
+            tmp_cl.unlink!
           end
         end
-
-        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
diff --git a/spec/unit/knife/cookbook_upload_spec.rb b/spec/unit/knife/cookbook_upload_spec.rb
index 0dde664513..6f87ffff39 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -183,7 +183,6 @@ describe Chef::Knife::CookbookUpload do
 
       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_uploader).not_to receive(:upload_cookbooks)
         expect { knife.run }.to raise_error(SystemExit)
       end