Merge pull request #9073 from MsysTechnologiesllc/vasundhara/generate_metadata_json_while_cookbook_upload

Generate metadata.json from metadata.rb if not exist before knife cookbook upload or knife upload or berkshelf upload

10 files changed, 413 insertions, 83 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 21b7cdaff8..52fc708552 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,12 +72,29 @@ class Chef
           end
 
           def upload_cookbook(other, options)
-            cookbook_to_upload = other.chef_object
-            cookbook_to_upload.freeze_version if options[:freeze]
-            uploader = Chef::CookbookUploader.new(cookbook_to_upload, force: options[:force], rest: chef_rest)
+            cookbook = other.chef_object if other.chef_object
+            raise Chef::Exceptions::MetadataNotFound.new(cookbook.root_paths[0], cookbook.name) unless cookbook.has_metadata_file?
 
-            with_actual_cookbooks_dir(other.parent.file_path) do
-              uploader.upload_cookbooks
+            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
           end
 
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/cookbook_version.rb b/lib/chef/cookbook_version.rb
index d546724fa0..4989fb8d91 100644
--- a/lib/chef/cookbook_version.rb
+++ b/lib/chef/cookbook_version.rb
@@ -445,6 +445,10 @@ class Chef
       end
     end
 
+    def has_metadata_file?
+      all_files.include?(metadata_json_file) || all_files.include?(metadata_rb_file)
+    end
+
     ##
     # REST API
     ##
@@ -513,6 +517,19 @@ class Chef
       @cookbook_manifest ||= CookbookManifest.new(self)
     end
 
+    def compile_metadata(path = root_dir)
+      json_file = "#{path}/metadata.json"
+      rb_file = "#{path}/metadata.rb"
+      return nil if File.exist?(json_file)
+
+      md = Chef::Cookbook::Metadata.new
+      md.from_file(rb_file)
+      f = File.open(json_file, "w")
+      f.write(Chef::JSONCompat.to_json_pretty(md))
+      f.close
+      f.path
+    end
+
     private
 
     def find_preferred_manifest_record(node, segment, filename)
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index d73fa9ae68..c8c9067800 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -20,11 +20,14 @@
 
 require_relative "../knife"
 require_relative "../cookbook_uploader"
+require_relative "../mixin/file_class"
 
 class Chef
   class Knife
     class CookbookUpload < Knife
 
+      include Chef::Mixin::FileClass
+
       CHECKSUM = "checksum".freeze
       MATCH_CHECKSUM = /[0-9a-f]{32,}/.freeze
 
@@ -93,56 +96,85 @@ 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
-        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
-              ui.warn("Not updating version constraints for some cookbooks in the environment as the cookbook is frozen.")
-            end
-            ui.info("Uploaded all cookbooks.")
-          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.")
+
+        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?
+
+          if cookbook.metadata.name.nil?
+            message = "Cookbook loaded at path [#{cookbook.root_paths[0]}] has invalid metadata: #{cookbook.metadata.errors.join("; ")}"
+            raise Chef::Exceptions::MetadataNotValid, message
           end
+
+          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
-          cookbooks_to_upload.each do |cookbook_name, cookbook|
-            cookbook.freeze_version if config[:freeze]
-            begin
-              upload([cookbook], justify_width)
-              upload_ok += 1
+          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
-            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
             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
-          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
+                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
 
-        unless version_constraints_to_update.empty?
-          update_version_constraints(version_constraints_to_update) if config[:environment]
+              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
+
+            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/data/cookbooks/apache2/metadata.json b/spec/data/cookbooks/apache2/metadata.json
new file mode 100644
index 0000000000..18f5e50bb3
--- /dev/null
+++ b/spec/data/cookbooks/apache2/metadata.json
@@ -0,0 +1,33 @@
+{
+  "name": "apache2",
+  "description": "",
+  "long_description": "",
+  "maintainer": "",
+  "maintainer_email": "",
+  "license": "All rights reserved",
+  "platforms": {
+
+  },
+  "dependencies": {
+
+  },
+  "providing": {
+
+  },
+  "recipes": {
+
+  },
+  "version": "0.0.1",
+  "source_url": "",
+  "issues_url": "",
+  "privacy": false,
+  "chef_versions": [
+
+  ],
+  "ohai_versions": [
+
+  ],
+  "gems": [
+
+  ]
+}
diff --git a/spec/data/cookbooks/java/metadata.json b/spec/data/cookbooks/java/metadata.json
new file mode 100644
index 0000000000..9d46842f3c
--- /dev/null
+++ b/spec/data/cookbooks/java/metadata.json
@@ -0,0 +1,33 @@
+{
+  "name": "java",
+  "description": "",
+  "long_description": "",
+  "maintainer": "",
+  "maintainer_email": "",
+  "license": "All rights reserved",
+  "platforms": {
+
+  },
+  "dependencies": {
+
+  },
+  "providing": {
+
+  },
+  "recipes": {
+
+  },
+  "version": "0.0.1",
+  "source_url": "",
+  "issues_url": "",
+  "privacy": false,
+  "chef_versions": [
+
+  ],
+  "ohai_versions": [
+
+  ],
+  "gems": [
+
+  ]
+}
diff --git a/spec/integration/knife/chef_fs_data_store_spec.rb b/spec/integration/knife/chef_fs_data_store_spec.rb
index 58ca5121c5..95fee18257 100644
--- a/spec/integration/knife/chef_fs_data_store_spec.rb
+++ b/spec/integration/knife/chef_fs_data_store_spec.rb
@@ -194,7 +194,7 @@ describe "ChefFSDataStore tests", :workstation do
             Uploading x              [1.0.0]
             Uploaded 1 cookbook.
           EOM
-          knife("list --local -Rfp /cookbooks").should_succeed "/cookbooks/x/\n/cookbooks/x/metadata.rb\n"
+          knife("list --local -Rfp /cookbooks").should_succeed "/cookbooks/x/\n/cookbooks/x/metadata.json\n/cookbooks/x/metadata.rb\n"
         end
 
         it "knife raw -z -i empty.json -m PUT /data/x/y" do
@@ -251,7 +251,7 @@ describe "ChefFSDataStore tests", :workstation do
             Uploading z            [1.0.0]
             Uploaded 1 cookbook.
           EOM
-          knife("list --local -Rfp /cookbooks").should_succeed "/cookbooks/z/\n/cookbooks/z/metadata.rb\n"
+          knife("list --local -Rfp /cookbooks").should_succeed "/cookbooks/z/\n/cookbooks/z/metadata.json\n/cookbooks/z/metadata.rb\n"
         end
 
         it "knife raw -z -i empty.json -m POST /data" do
diff --git a/spec/integration/knife/cookbook_upload_spec.rb b/spec/integration/knife/cookbook_upload_spec.rb
index 7e98b6ea64..6496a911b0 100644
--- a/spec/integration/knife/cookbook_upload_spec.rb
+++ b/spec/integration/knife/cookbook_upload_spec.rb
@@ -86,5 +86,15 @@ describe "knife cookbook upload", :workstation do
         EOM
       end
     end
+
+    when_the_repository "has cookbook metadata without name attribute in metadata file" do
+      before do
+        file "cookbooks/x/metadata.rb", cb_metadata(nil, "1.0.0")
+      end
+
+      it "knife cookbook upload x " do
+        expect { knife("cookbook upload x -o #{cb_dir}") }.to raise_error(Chef::Exceptions::MetadataNotValid)
+      end
+    end
   end
 end
diff --git a/spec/integration/knife/upload_spec.rb b/spec/integration/knife/upload_spec.rb
index 1a6ddceb17..77db50fc22 100644
--- a/spec/integration/knife/upload_spec.rb
+++ b/spec/integration/knife/upload_spec.rb
@@ -209,7 +209,10 @@ describe "knife upload", :workstation do
               Created /roles/y.json
               Created /users/y.json
             EOM
-            knife("diff /").should_succeed ""
+            knife("diff --name-status /").should_succeed <<~EOM
+              D\t/cookbooks/x/metadata.json
+              D\t/cookbooks/y/metadata.json
+            EOM
           end
 
           it "knife upload --no-diff adds the new files" do
@@ -225,7 +228,10 @@ describe "knife upload", :workstation do
               Created /roles/y.json
               Created /users/y.json
             EOM
-            knife("diff --name-status /").should_succeed ""
+            knife("diff --name-status /").should_succeed <<~EOM
+              D\t/cookbooks/x/metadata.json
+              D\t/cookbooks/y/metadata.json
+            EOM
           end
         end
       end
@@ -289,8 +295,8 @@ describe "knife upload", :workstation do
             Created /data_bags/x
             Created /data_bags/x/y.json
           EOM
-          knife("diff --name-status /data_bags").should_succeed <<EOM
-EOM
+          knife("diff --name-status /data_bags").should_succeed <<~EOM
+          EOM
           expect(Chef::JSONCompat.parse(knife("raw /data/x/y").stdout, create_additions: false).keys.sort).to eq(%w{foo id})
         end
 
@@ -446,11 +452,30 @@ EOM
     # upload of a file is designed not to work at present.  Make sure that is the
     # case.
     when_the_chef_server "has a cookbook" do
-
       before do
         cookbook "x", "1.0.0", { "z.rb" => "" }
       end
 
+      when_the_repository "does not have metadata file" do
+        before do
+          file "cookbooks/x/y.rb", "hi"
+        end
+
+        it "raises MetadataNotFound exception" do
+          expect { knife("upload /cookbooks/x") }.to raise_error(Chef::Exceptions::MetadataNotFound)
+        end
+      end
+
+      when_the_repository "does not have valid metadata" do
+        before do
+          file "cookbooks/x/metadata.rb", cb_metadata(nil, "1.0.0")
+        end
+
+        it "raises exception for invalid metadata" do
+          expect { knife("upload /cookbooks/x") }.to raise_error(Chef::Exceptions::MetadataNotValid)
+        end
+      end
+
       when_the_repository "has a modified, extra and missing file for the cookbook" do
         before do
           file "cookbooks/x/metadata.rb", cb_metadata("x", "1.0.0", "#modified")
@@ -462,6 +487,7 @@ EOM
           knife("upload /cookbooks/x/y.rb").should_fail "ERROR: /cookbooks/x cannot have a child created under it.\n"
           knife("upload --purge /cookbooks/x/z.rb").should_fail "ERROR: /cookbooks/x/z.rb cannot be deleted.\n"
         end
+
         # TODO this is a bit of an inconsistency: if we didn't specify --purge,
         # technically we shouldn't have deleted missing files.  But ... cookbooks
         # are a special case.
@@ -469,13 +495,18 @@ EOM
           knife("upload /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
+
         it "knife upload --purge of the cookbook itself succeeds" do
           knife("upload /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
       when_the_repository "has a missing file for the cookbook" do
@@ -488,7 +519,9 @@ EOM
           knife("upload /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
       when_the_repository "has an extra file for the cookbook" do
@@ -503,7 +536,9 @@ EOM
           knife("upload /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
 
@@ -548,6 +583,7 @@ EOM
     when_the_repository "has a cookbook" do
       before do
         file "cookbooks/x/metadata.rb", cb_metadata("x", "1.0.0")
+        file "cookbooks/x/metadata.json", { name: "x", version: "1.0.0" }
         file "cookbooks/x/onlyin1.0.0.rb", "old_text"
       end
 
@@ -561,6 +597,38 @@ EOM
           knife("diff --name-status /cookbooks").should_succeed <<~EOM
             M\t/cookbooks/x/metadata.rb
             D\t/cookbooks/x/onlyin1.0.1.rb
+            A\t/cookbooks/x/metadata.json
+            A\t/cookbooks/x/onlyin1.0.0.rb
+          EOM
+          knife("upload --purge /cookbooks/x").should_succeed <<~EOM
+            Updated /cookbooks/x
+          EOM
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            M\t/cookbooks/x/metadata.rb
+            D\t/cookbooks/x/onlyin1.0.1.rb
+            A\t/cookbooks/x/metadata.json
+            A\t/cookbooks/x/onlyin1.0.0.rb
+          EOM
+        end
+      end
+    end
+
+    when_the_repository "has a cookbook" do
+      before do
+        file "cookbooks/x/metadata.rb", cb_metadata("x", "1.0.0")
+        file "cookbooks/x/onlyin1.0.0.rb", "old_text"
+      end
+
+      when_the_chef_server "has a later version for the cookbook" do
+        before do
+          cookbook "x", "1.0.0", { "onlyin1.0.0.rb" => "" }
+          cookbook "x", "1.0.1", { "onlyin1.0.1.rb" => "hi" }
+        end
+
+        it "knife upload /cookbooks/x uploads the local version and generates metadata.json from metadata.rb and uploads it." do
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            M\t/cookbooks/x/metadata.rb
+            D\t/cookbooks/x/onlyin1.0.1.rb
             A\t/cookbooks/x/onlyin1.0.0.rb
           EOM
           knife("upload --purge /cookbooks/x").should_succeed <<~EOM
@@ -580,11 +648,13 @@ EOM
           cookbook "x", "0.9.9", { "onlyin0.9.9.rb" => "hi" }
         end
 
-        it "knife upload /cookbooks/x uploads the local version" do
+        it "knife upload /cookbooks/x uploads the local version generates metadata.json and uploads it." do
           knife("upload --purge /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
 
@@ -593,7 +663,7 @@ EOM
           cookbook "x", "1.0.1", { "onlyin1.0.1.rb" => "hi" }
         end
 
-        it "knife upload /cookbooks/x uploads the local version" do
+        it "knife upload /cookbooks/x uploads the local version and generates metadata.json before upload and uploads it." do
           knife("diff --name-status /cookbooks").should_succeed <<~EOM
             M\t/cookbooks/x/metadata.rb
             D\t/cookbooks/x/onlyin1.0.1.rb
@@ -619,7 +689,9 @@ EOM
           knife("upload --purge /cookbooks/x").should_succeed <<~EOM
             Updated /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
     end
@@ -719,7 +791,9 @@ EOM
           knife("upload /cookbooks/x").should_succeed <<~EOM
             Created /cookbooks/x
           EOM
-          knife("diff --name-status /cookbooks").should_succeed ""
+          knife("diff --name-status /cookbooks").should_succeed <<~EOM
+            D\t/cookbooks/x/metadata.json
+          EOM
         end
       end
     end
diff --git a/spec/unit/knife/cookbook_upload_spec.rb b/spec/unit/knife/cookbook_upload_spec.rb
index 9c371c4140..6bbd6525da 100644
--- a/spec/unit/knife/cookbook_upload_spec.rb
+++ b/spec/unit/knife/cookbook_upload_spec.rb
@@ -23,7 +23,12 @@ require "chef/cookbook_uploader"
 require "timeout"
 
 describe Chef::Knife::CookbookUpload do
-  let(:cookbook) { Chef::CookbookVersion.new("test_cookbook", "/tmp/blah.txt") }
+  let(:cookbook) do
+    cookbook = Chef::CookbookVersion.new("test_cookbook", "/tmp/blah")
+    allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+    allow(cookbook.metadata).to receive(:name).and_return(cookbook.name)
+    cookbook
+  end
 
   let(:cookbooks_by_name) do
     { cookbook.name => cookbook }
@@ -33,6 +38,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,6 +60,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)
   end
 
   describe "with --concurrency" do
@@ -61,6 +70,7 @@ 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))
@@ -81,6 +91,34 @@ describe Chef::Knife::CookbookUpload do
       expect { knife.run }.to raise_error(SystemExit)
     end
 
+    describe "when specifying cookbook without metadata.rb or metadata.json" do
+      let(:name_args) { ["test_cookbook1"] }
+      let(:cookbook) do
+        cookbook = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
+        allow(cookbook).to receive(:has_metadata_file?).and_return(false)
+        cookbook
+      end
+
+      it "should upload the cookbook" do
+        expect { knife.run }.to raise_error(Chef::Exceptions::MetadataNotFound)
+      end
+    end
+
+    describe "when name attribute in metadata not set" do
+      let(:name_args) { ["test_cookbook1"] }
+
+      let(:cookbook) do
+        cookbook = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
+        allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+        allow(cookbook.metadata).to receive(:name).and_return(nil)
+        cookbook
+      end
+
+      it "should upload the cookbook" do
+        expect { knife.run }.to raise_error(Chef::Exceptions::MetadataNotValid)
+      end
+    end
+
     describe "when specifying a cookbook name" do
       it "should upload the cookbook" do
         expect(knife).to receive(:upload).once
@@ -105,12 +143,15 @@ describe Chef::Knife::CookbookUpload do
     describe "when specifying a cookbook name among many" do
       let(:name_args) { ["test_cookbook1"] }
 
+      let(:cookbook) do
+        cookbook = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
+        allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+        allow(cookbook.metadata).to receive(:name).and_return(cookbook.name)
+        cookbook
+      end
+
       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"),
-        }
+        { cookbook.name => cookbook }
       end
 
       it "should read only one cookbook" do
@@ -119,7 +160,7 @@ describe Chef::Knife::CookbookUpload do
       end
 
       it "should not read all cookbooks" do
-        expect(cookbook_loader).not_to receive(:load_cookbooks)
+        expect(cookbook_loader).to receive(:load_cookbooks)
         knife.run
       end
 
@@ -133,17 +174,18 @@ describe Chef::Knife::CookbookUpload do
     describe "when specifying a cookbook name with dependencies" do
       let(:name_args) { ["test_cookbook2"] }
 
-      let(:cookbooks_by_name) do
-        { "test_cookbook1" => test_cookbook1,
-          "test_cookbook2" => test_cookbook2,
-          "test_cookbook3" => test_cookbook3 }
+      let(:test_cookbook1) do
+        cookbook = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
+        allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+        allow(cookbook.metadata).to receive(:name).and_return(cookbook.name)
+        cookbook
       end
 
-      let(:test_cookbook1) { Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah") }
-
       let(:test_cookbook2) do
         c = Chef::CookbookVersion.new("test_cookbook2")
         c.metadata.depends("test_cookbook3")
+        allow(c).to receive(:has_metadata_file?).and_return(true)
+        allow(c.metadata).to receive(:name).and_return(c.name)
         c
       end
 
@@ -151,9 +193,17 @@ describe Chef::Knife::CookbookUpload do
         c = Chef::CookbookVersion.new("test_cookbook3")
         c.metadata.depends("test_cookbook1")
         c.metadata.depends("test_cookbook2")
+        allow(c).to receive(:has_metadata_file?).and_return(true)
+        allow(c.metadata).to receive(:name).and_return(c.name)
         c
       end
 
+      let(:cookbooks_by_name) do
+        { "test_cookbook1" => test_cookbook1,
+          "test_cookbook2" => test_cookbook2,
+          "test_cookbook3" => test_cookbook3 }
+      end
+
       it "should upload all dependencies once" do
         knife.config[:depends] = true
         allow(knife).to receive(:cookbook_names).and_return(%w{test_cookbook1 test_cookbook2 test_cookbook3})
@@ -182,7 +232,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
@@ -214,7 +263,7 @@ describe Chef::Knife::CookbookUpload do
 
     it "should freeze the version of the cookbooks if --freeze is specified" do
       knife.config[:freeze] = true
-      expect(cookbook).to receive(:freeze_version).once
+      expect(cookbook_loader).to receive(:freeze_versions).once
       knife.run
     end
 
@@ -224,10 +273,22 @@ describe Chef::Knife::CookbookUpload do
       end
 
       context "when cookbooks exist in the cookbook path" do
+        let(:test_cookbook1) do
+          cookbook = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
+          allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+          allow(cookbook.metadata).to receive(:name).and_return(cookbook.name)
+          cookbook
+        end
+
+        let(:test_cookbook2) do
+          cookbook = Chef::CookbookVersion.new("test_cookbook2", "/tmp/blah")
+          allow(cookbook).to receive(:has_metadata_file?).and_return(true)
+          allow(cookbook.metadata).to receive(:name).and_return(cookbook.name)
+          cookbook
+        end
+
         before(:each) do
-          @test_cookbook1 = Chef::CookbookVersion.new("test_cookbook1", "/tmp/blah")
-          @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(: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})
         end