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

6 files changed, 245 insertions, 34 deletions

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