Merge branch '3618-run-list-mutation-issue'

2 files changed, 121 insertions, 53 deletions

diff --git a/lib/chef/run_list/versioned_recipe_list.rb b/lib/chef/run_list/versioned_recipe_list.rb
index 7cce6fa48c..2824f08f31 100644
--- a/lib/chef/run_list/versioned_recipe_list.rb
+++ b/lib/chef/run_list/versioned_recipe_list.rb
@@ -70,15 +70,16 @@ class Chef
       # @return [Array] Array of strings with fully-qualified recipe names
       def with_fully_qualified_names_and_version_constraints
         self.map do |recipe_name|
-          ret = if recipe_name.include?('::')
+          qualified_recipe = if recipe_name.include?('::')
             recipe_name
           else
             "#{recipe_name}::default"
           end
-          if @versions[recipe_name]
-            ret << "@#{@versions[recipe_name]}"
-          end
-          ret
+
+          version = @versions[recipe_name]
+          qualified_recipe = "#{qualified_recipe}@#{version}" if version
+
+          qualified_recipe
         end
       end
     end
diff --git a/spec/unit/run_list/versioned_recipe_list_spec.rb b/spec/unit/run_list/versioned_recipe_list_spec.rb
index 209ac37fc1..9c3ecaa0dd 100644
--- a/spec/unit/run_list/versioned_recipe_list_spec.rb
+++ b/spec/unit/run_list/versioned_recipe_list_spec.rb
@@ -26,98 +26,165 @@ describe Chef::RunList::VersionedRecipeList do
     end
   end
 
+  let(:list) { described_class.new }
+
+  let(:versioned_recipes) { [] }
+
+  let(:recipes) { [] }
+
+  before do
+    recipes.each { |r| list << r }
+    versioned_recipes.each {|r| list.add_recipe r[:name], r[:version]}
+  end
+
   describe "add_recipe" do
-    before(:each) do
-      @list = Chef::RunList::VersionedRecipeList.new
-      @list << "apt"
-      @list << "god"
-      @list << "apache2"
-    end
+
+    let(:recipes) { %w[ apt god apache2 ] }
 
     it "should append the recipe to the end of the list" do
-      @list.add_recipe "rails"
-      expect(@list).to eq(["apt", "god", "apache2", "rails"])
+      list.add_recipe "rails"
+      expect(list).to eq(["apt", "god", "apache2", "rails"])
     end
 
     it "should not duplicate entries" do
-      @list.add_recipe "apt"
-      expect(@list).to eq(["apt", "god", "apache2"])
+      list.add_recipe "apt"
+      expect(list).to eq(["apt", "god", "apache2"])
     end
 
     it "should allow you to specify a version" do
-      @list.add_recipe "rails", "1.0.0"
-      expect(@list).to eq(["apt", "god", "apache2", "rails"])
-      expect(@list.with_versions).to include({:name => "rails", :version => "1.0.0"})
+      list.add_recipe "rails", "1.0.0"
+      expect(list).to eq(["apt", "god", "apache2", "rails"])
+      expect(list.with_versions).to include({:name => "rails", :version => "1.0.0"})
     end
 
     it "should allow you to specify a version for a recipe that already exists" do
-      @list.add_recipe "apt", "1.2.3"
-      expect(@list).to eq(["apt", "god", "apache2"])
-      expect(@list.with_versions).to include({:name => "apt", :version => "1.2.3"})
+      list.add_recipe "apt", "1.2.3"
+      expect(list).to eq(["apt", "god", "apache2"])
+      expect(list.with_versions).to include({:name => "apt", :version => "1.2.3"})
     end
 
     it "should allow you to specify the same version of a recipe twice" do
-      @list.add_recipe "rails", "1.0.0"
-      @list.add_recipe "rails", "1.0.0"
-      expect(@list.with_versions).to include({:name => "rails", :version => "1.0.0"})
+      list.add_recipe "rails", "1.0.0"
+      list.add_recipe "rails", "1.0.0"
+      expect(list.with_versions).to include({:name => "rails", :version => "1.0.0"})
     end
 
     it "should allow you to spcify no version, even when a version already exists" do
-      @list.add_recipe "rails", "1.0.0"
-      @list.add_recipe "rails"
-      expect(@list.with_versions).to include({:name => "rails", :version => "1.0.0"})
+      list.add_recipe "rails", "1.0.0"
+      list.add_recipe "rails"
+      expect(list.with_versions).to include({:name => "rails", :version => "1.0.0"})
     end
 
     it "should not allow multiple versions of the same recipe" do
-      @list.add_recipe "rails", "1.0.0"
-      expect {@list.add_recipe "rails", "0.1.0"}.to raise_error Chef::Exceptions::CookbookVersionConflict
+      list.add_recipe "rails", "1.0.0"
+      expect {list.add_recipe "rails", "0.1.0"}.to raise_error Chef::Exceptions::CookbookVersionConflict
     end
   end
 
   describe "with_versions" do
-    before(:each) do
-      @recipes = [
+
+    let(:versioned_recipes) do
+      [
         {:name => "apt", :version => "1.0.0"},
         {:name => "god", :version => nil},
         {:name => "apache2", :version => "0.0.1"}
       ]
-      @list = Chef::RunList::VersionedRecipeList.new
-      @recipes.each {|i| @list.add_recipe i[:name], i[:version]}
     end
-
     it "should return an array of hashes with :name and :version" do
-      expect(@list.with_versions).to eq(@recipes)
+      expect(list.with_versions).to eq(versioned_recipes)
     end
 
     it "should retain the same order as the version-less list" do
-      with_versions = @list.with_versions
-      @list.each_with_index do |item, index|
+      with_versions = list.with_versions
+      list.each_with_index do |item, index|
         expect(with_versions[index][:name]).to eq(item)
       end
     end
   end
 
   describe "with_version_constraints" do
-    before(:each) do
-      @recipes = [
-                  {:name => "apt", :version => "~> 1.2.0"},
-                  {:name => "god", :version => nil},
-                  {:name => "apache2", :version => "0.0.1"}
-                 ]
-      @list = Chef::RunList::VersionedRecipeList.new
-      @recipes.each {|i| @list.add_recipe i[:name], i[:version]}
-      @constraints = @recipes.map do |x|
-        { :name => x[:name],
-          :version_constraint => Chef::VersionConstraint.new(x[:version])
-        }
-      end
+
+    let(:versioned_recipes) do
+      [
+        {:name => "apt", :version => "~> 1.2.0"},
+        {:name => "god", :version => nil},
+        {:name => "apache2", :version => "0.0.1"}
+      ]
     end
 
+
     it "should return an array of hashes with :name and :version_constraint" do
-      @list.with_version_constraints.each do |x|
-        expect(x).to have_key :name
-        expect(x[:version_constraint]).not_to be nil
+      list.with_version_constraints.each_with_index do |recipe_spec, i|
+
+        expected_recipe = versioned_recipes[i]
+
+        expect(recipe_spec[:name]).to eq(expected_recipe[:name])
+        expect(recipe_spec[:version_constraint]).to eq(Chef::VersionConstraint.new(expected_recipe[:version]))
       end
     end
   end
+
+  describe "with_fully_qualified_names_and_version_constraints" do
+
+    let(:fq_names) { list.with_fully_qualified_names_and_version_constraints }
+
+    context "with bare cookbook names" do
+
+      let(:recipes) { %w[ apache2 ] }
+
+      it "gives $cookbook_name::default" do
+        expect(fq_names).to eq( %w[ apache2::default ] )
+      end
+
+    end
+
+    context "with qualified recipe names but no versions" do
+
+      let(:recipes) { %w[ mysql::server ] }
+
+      it "returns the qualified recipe names" do
+        expect(fq_names).to eq( %w[ mysql::server ] )
+      end
+
+    end
+
+    context "with unqualified names that have version constraints" do
+
+      let(:versioned_recipes) do
+        [
+          {:name => "apt", :version => "~> 1.2.0"},
+        ]
+      end
+
+      it "gives qualified names with their versions" do
+        expect(fq_names).to eq([ "apt::default@~> 1.2.0" ])
+      end
+
+      it "does not mutate the recipe name" do
+        expect(fq_names).to eq([ "apt::default@~> 1.2.0" ])
+        expect(list).to eq( [ "apt" ] )
+      end
+
+    end
+
+    context "with fully qualified names that have version constraints" do
+
+      let(:versioned_recipes) do
+        [
+          {:name => "apt::cacher", :version => "~> 1.2.0"},
+        ]
+      end
+
+      it "gives qualified names with their versions" do
+        expect(fq_names).to eq([ "apt::cacher@~> 1.2.0" ])
+      end
+
+      it "does not mutate the recipe name" do
+        expect(fq_names).to eq([ "apt::cacher@~> 1.2.0" ])
+        expect(list).to eq( [ "apt::cacher" ] )
+      end
+
+    end
+  end
+
 end