Merge pull request #2179 from opscode/sersut/port-policyfile-fix

Make FileVendor configuration specific to the two implementations

17 files changed, 128 insertions, 23 deletions

diff --git a/lib/chef/cookbook/file_system_file_vendor.rb b/lib/chef/cookbook/file_system_file_vendor.rb
index 8896e3ed30..e351ec4702 100644
--- a/lib/chef/cookbook/file_system_file_vendor.rb
+++ b/lib/chef/cookbook/file_system_file_vendor.rb
@@ -31,6 +31,9 @@ class Chef
     # non-sensical.
     class FileSystemFileVendor < FileVendor
 
+      attr_reader :cookbook_name
+      attr_reader :repo_paths
+
       def initialize(manifest, *repo_paths)
         @cookbook_name = manifest[:cookbook_name]
         @repo_paths = repo_paths.flatten
diff --git a/lib/chef/cookbook/file_vendor.rb b/lib/chef/cookbook/file_vendor.rb
index 406f23ca25..b82b52f90c 100644
--- a/lib/chef/cookbook/file_vendor.rb
+++ b/lib/chef/cookbook/file_vendor.rb
@@ -24,15 +24,39 @@ class Chef
     # This class handles fetching of cookbook files based on specificity.
     class FileVendor
 
-      def self.on_create(&block)
-        @instance_creator = block
+      @vendor_class = nil
+      @initialization_options = nil
+
+      # Configures FileVendor to use the RemoteFileVendor implementation. After
+      # calling this, subsequent calls to create_from_manifest will return a
+      # RemoteFileVendor object initialized with the given http_client
+      def self.fetch_from_remote(http_client)
+        @vendor_class = RemoteFileVendor
+        @initialization_options = http_client
+      end
+
+      def self.fetch_from_disk(cookbook_paths)
+        @vendor_class = FileSystemFileVendor
+        @initialization_options = cookbook_paths
+      end
+
+      # Returns the implementation class that is currently configured, or `nil`
+      # if one has not been configured yet.
+      def self.vendor_class
+        @vendor_class
+      end
+
+      def self.initialization_options
+        @initialization_options
       end
 
       # Factory method that creates the appropriate kind of
       # Cookbook::FileVendor to serve the contents of the manifest
       def self.create_from_manifest(manifest)
-        raise "Must call Chef::Cookbook::FileVendor.on_create before calling create_from_manifest factory" unless defined?(@instance_creator)
-        @instance_creator.call(manifest)
+        if @vendor_class.nil?
+          raise "Must configure FileVendor to use a specific implementation before creating an instance"
+        end
+        @vendor_class.new(manifest, @initialization_options)
       end
 
       # Gets the on-disk location for the given cookbook file.
diff --git a/lib/chef/cookbook/remote_file_vendor.rb b/lib/chef/cookbook/remote_file_vendor.rb
index 49de62cf65..2ddce31001 100644
--- a/lib/chef/cookbook/remote_file_vendor.rb
+++ b/lib/chef/cookbook/remote_file_vendor.rb
@@ -25,6 +25,9 @@ class Chef
     # if not available, loading them from the remote server.
     class RemoteFileVendor < FileVendor
 
+      attr_reader :rest
+      attr_reader :cookbook_name
+
       def initialize(manifest, rest)
         @manifest = manifest
         @cookbook_name = @manifest[:cookbook_name]
diff --git a/lib/chef/knife/cookbook_upload.rb b/lib/chef/knife/cookbook_upload.rb
index 9d6e0d438d..4b2b611171 100644
--- a/lib/chef/knife/cookbook_upload.rb
+++ b/lib/chef/knife/cookbook_upload.rb
@@ -184,7 +184,7 @@ class Chef
 
       def cookbook_repo
         @cookbook_loader ||= begin
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, config[:cookbook_path]) }
+          Chef::Cookbook::FileVendor.fetch_from_disk(config[:cookbook_path])
           Chef::CookbookLoader.new(config[:cookbook_path])
         end
       end
diff --git a/lib/chef/policy_builder/expand_node_object.rb b/lib/chef/policy_builder/expand_node_object.rb
index 161e14ca38..6e4fa84569 100644
--- a/lib/chef/policy_builder/expand_node_object.rb
+++ b/lib/chef/policy_builder/expand_node_object.rb
@@ -56,13 +56,13 @@ class Chef
 
       def setup_run_context(specific_recipes=nil)
         if Chef::Config[:solo]
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, Chef::Config[:cookbook_path]) }
+          Chef::Cookbook::FileVendor.fetch_from_disk(Chef::Config[:cookbook_path])
           cl = Chef::CookbookLoader.new(Chef::Config[:cookbook_path])
           cl.load_cookbooks
           cookbook_collection = Chef::CookbookCollection.new(cl)
           run_context = Chef::RunContext.new(node, cookbook_collection, @events)
         else
-          Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, api_service) }
+          Chef::Cookbook::FileVendor.fetch_from_remote(api_service)
           cookbook_hash = sync_cookbooks
           cookbook_collection = Chef::CookbookCollection.new(cookbook_hash)
           run_context = Chef::RunContext.new(node, cookbook_collection, @events)
diff --git a/lib/chef/policy_builder/policyfile.rb b/lib/chef/policy_builder/policyfile.rb
index 4baa6340d4..0df3dd5dd2 100644
--- a/lib/chef/policy_builder/policyfile.rb
+++ b/lib/chef/policy_builder/policyfile.rb
@@ -154,10 +154,7 @@ class Chef
       end
 
       def setup_run_context(specific_recipes=nil)
-        # TODO: This file vendor stuff is duplicated and initializing it with a
-        # block traps a reference to this object in a global context which will
-        # prevent it from getting GC'd. Simplify it.
-        Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, api_service) }
+        Chef::Cookbook::FileVendor.fetch_from_remote(http_api)
         sync_cookbooks
         cookbook_collection = Chef::CookbookCollection.new(cookbooks_to_sync)
         run_context = Chef::RunContext.new(node, cookbook_collection, events)
diff --git a/lib/chef/shell/shell_session.rb b/lib/chef/shell/shell_session.rb
index a158020116..73e6c34ebb 100644
--- a/lib/chef/shell/shell_session.rb
+++ b/lib/chef/shell/shell_session.rb
@@ -169,7 +169,7 @@ module Shell
 
     def rebuild_context
       @run_status = Chef::RunStatus.new(@node, @events)
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, Chef::Config[:cookbook_path]) }
+      Chef::Cookbook::FileVendor.fetch_from_disk(Chef::Config[:cookbook_path])
       cl = Chef::CookbookLoader.new(Chef::Config[:cookbook_path])
       cl.load_cookbooks
       cookbook_collection = Chef::CookbookCollection.new(cl)
@@ -201,7 +201,7 @@ module Shell
 
     def rebuild_context
       @run_status = Chef::RunStatus.new(@node, @events)
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::RemoteFileVendor.new(manifest, Chef::REST.new(Chef::Config[:server_url])) }
+      Chef::Cookbook::FileVendor.fetch_from_remote(Chef::REST.new(Chef::Config[:chef_server_url]))
       cookbook_hash = @client.sync_cookbooks
       cookbook_collection = Chef::CookbookCollection.new(cookbook_hash)
       @run_context = Chef::RunContext.new(node, cookbook_collection, @events)
diff --git a/spec/functional/resource/cookbook_file_spec.rb b/spec/functional/resource/cookbook_file_spec.rb
index 173dac8268..7797ed0041 100644
--- a/spec/functional/resource/cookbook_file_spec.rb
+++ b/spec/functional/resource/cookbook_file_spec.rb
@@ -40,7 +40,7 @@ describe Chef::Resource::CookbookFile do
     # set up cookbook collection for this run to use, based on our
     # spec data.
     cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, 'cookbooks'))
-    Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, cookbook_repo) }
+    Chef::Cookbook::FileVendor.fetch_from_disk(cookbook_repo)
     loader = Chef::CookbookLoader.new(cookbook_repo)
     loader.load_cookbooks
     cookbook_collection = Chef::CookbookCollection.new(loader)
diff --git a/spec/functional/resource/package_spec.rb b/spec/functional/resource/package_spec.rb
index 797f308cf9..548db40e79 100644
--- a/spec/functional/resource/package_spec.rb
+++ b/spec/functional/resource/package_spec.rb
@@ -135,9 +135,7 @@ describe Chef::Resource::Package, metadata do
       cookbook_path = File.join(CHEF_SPEC_DATA, "cookbooks")
       cl = Chef::CookbookLoader.new(cookbook_path)
       cl.load_cookbooks
-      Chef::Cookbook::FileVendor.on_create do |manifest|
-        Chef::Cookbook::FileSystemFileVendor.new(manifest, cookbook_path)
-      end
+      Chef::Cookbook::FileVendor.fetch_from_disk(cookbook_path)
       Chef::CookbookCollection.new(cl)
     end
 
diff --git a/spec/functional/resource/remote_directory_spec.rb b/spec/functional/resource/remote_directory_spec.rb
index 70e575abb0..f9eb20711e 100644
--- a/spec/functional/resource/remote_directory_spec.rb
+++ b/spec/functional/resource/remote_directory_spec.rb
@@ -26,7 +26,7 @@ describe Chef::Resource::RemoteDirectory do
 
   def create_resource
     cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks"))
-    Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, cookbook_repo) }
+    Chef::Cookbook::FileVendor.fetch_from_disk(cookbook_repo)
     node = Chef::Node.new
     cl = Chef::CookbookLoader.new(cookbook_repo)
     cl.load_cookbooks
diff --git a/spec/functional/resource/template_spec.rb b/spec/functional/resource/template_spec.rb
index 7816d6357b..fefd995743 100644
--- a/spec/functional/resource/template_spec.rb
+++ b/spec/functional/resource/template_spec.rb
@@ -37,7 +37,7 @@ describe Chef::Resource::Template do
 
   def create_resource
     cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks"))
-    Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, cookbook_repo) }
+    Chef::Cookbook::FileVendor.fetch_from_disk(cookbook_repo)
     cl = Chef::CookbookLoader.new(cookbook_repo)
     cl.load_cookbooks
     cookbook_collection = Chef::CookbookCollection.new(cl)
diff --git a/spec/unit/cookbook/file_vendor_spec.rb b/spec/unit/cookbook/file_vendor_spec.rb
new file mode 100644
index 0000000000..4fad7d5808
--- /dev/null
+++ b/spec/unit/cookbook/file_vendor_spec.rb
@@ -0,0 +1,78 @@
+#--
+# Author:: Daniel DeLeo (<dan@getchef.com>)
+# Copyright:: Copyright (c) 2014 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+require 'spec_helper'
+
+describe Chef::Cookbook::FileVendor do
+
+  let(:file_vendor_class) { Class.new(described_class) }
+
+  # A manifest is a Hash of the format defined by Chef::CookbookVersion#manifest
+  let(:manifest) { {:cookbook_name => "bob"} }
+
+  context "when configured to fetch files over http" do
+
+    let(:http) { double("Chef::REST") }
+
+    before do
+      file_vendor_class.fetch_from_remote(http)
+    end
+
+    it "sets the vendor class to RemoteFileVendor" do
+      expect(file_vendor_class.vendor_class).to eq(Chef::Cookbook::RemoteFileVendor)
+    end
+
+    it "sets the initialization options to the given http object" do
+      expect(file_vendor_class.initialization_options).to eq(http)
+    end
+
+    it "creates a RemoteFileVendor for a given manifest" do
+      file_vendor = file_vendor_class.create_from_manifest(manifest)
+      expect(file_vendor).to be_a_kind_of(Chef::Cookbook::RemoteFileVendor)
+      expect(file_vendor.rest).to eq(http)
+      expect(file_vendor.cookbook_name).to eq("bob")
+    end
+
+  end
+
+  context "when configured to load files from disk" do
+
+    let(:cookbook_path) { %w[/var/chef/cookbooks /var/chef/other_cookbooks] }
+
+    before do
+      file_vendor_class.fetch_from_disk(cookbook_path)
+    end
+
+    it "sets the vendor class to FileSystemFileVendor" do
+      expect(file_vendor_class.vendor_class).to eq(Chef::Cookbook::FileSystemFileVendor)
+    end
+
+    it "sets the initialization options to the given cookbook paths" do
+      expect(file_vendor_class.initialization_options).to eq(cookbook_path)
+    end
+
+    it "creates a FileSystemFileVendor for a given manifest" do
+      file_vendor = file_vendor_class.create_from_manifest(manifest)
+      expect(file_vendor).to be_a_kind_of(Chef::Cookbook::FileSystemFileVendor)
+      expect(file_vendor.cookbook_name).to eq("bob")
+      expect(file_vendor.repo_paths).to eq(cookbook_path)
+    end
+
+  end
+
+end
+
diff --git a/spec/unit/mixin/template_spec.rb b/spec/unit/mixin/template_spec.rb
index 3aa0b9ba22..63fa81782e 100644
--- a/spec/unit/mixin/template_spec.rb
+++ b/spec/unit/mixin/template_spec.rb
@@ -76,7 +76,7 @@ describe Chef::Mixin::Template, "render_template" do
   describe "with a template resource" do
     before :each do
       @cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks"))
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, @cookbook_repo) }
+      Chef::Cookbook::FileVendor.fetch_from_disk(@cookbook_repo)
 
       @node = Chef::Node.new
       cl = Chef::CookbookLoader.new(@cookbook_repo)
diff --git a/spec/unit/provider/package_spec.rb b/spec/unit/provider/package_spec.rb
index 33b4f8186a..375a0d0646 100644
--- a/spec/unit/provider/package_spec.rb
+++ b/spec/unit/provider/package_spec.rb
@@ -339,7 +339,7 @@ describe Chef::Provider::Package do
   describe "when given a response file" do
     before(:each) do
       @cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks"))
-      Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, @cookbook_repo) }
+      Chef::Cookbook::FileVendor.fetch_from_disk(@cookbook_repo)
 
       @node = Chef::Node.new
       cl = Chef::CookbookLoader.new(@cookbook_repo)
diff --git a/spec/unit/provider/remote_directory_spec.rb b/spec/unit/provider/remote_directory_spec.rb
index d3f1438f0f..b986e2c8ad 100644
--- a/spec/unit/provider/remote_directory_spec.rb
+++ b/spec/unit/provider/remote_directory_spec.rb
@@ -35,7 +35,7 @@ describe Chef::Provider::RemoteDirectory do
     @resource.cookbook('openldap')
 
     @cookbook_repo = ::File.expand_path(::File.join(CHEF_SPEC_DATA, "cookbooks"))
-    Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, @cookbook_repo) }
+    Chef::Cookbook::FileVendor.fetch_from_disk(@cookbook_repo)
 
     @node = Chef::Node.new
     cl = Chef::CookbookLoader.new(@cookbook_repo)
diff --git a/spec/unit/provider/template/content_spec.rb b/spec/unit/provider/template/content_spec.rb
index dfc5d21c2a..b419e70519 100644
--- a/spec/unit/provider/template/content_spec.rb
+++ b/spec/unit/provider/template/content_spec.rb
@@ -36,7 +36,7 @@ describe Chef::Provider::Template::Content do
 
   let(:run_context) do
     cookbook_repo = File.expand_path(File.join(CHEF_SPEC_DATA, "cookbooks"))
-    Chef::Cookbook::FileVendor.on_create { |manifest| Chef::Cookbook::FileSystemFileVendor.new(manifest, cookbook_repo) }
+    Chef::Cookbook::FileVendor.fetch_from_disk(cookbook_repo)
     cl = Chef::CookbookLoader.new(cookbook_repo)
     cl.load_cookbooks
     cookbook_collection = Chef::CookbookCollection.new(cl)
diff --git a/spec/unit/shell/shell_session_spec.rb b/spec/unit/shell/shell_session_spec.rb
index 92a2e5d538..f49c9fc805 100644
--- a/spec/unit/shell/shell_session_spec.rb
+++ b/spec/unit/shell/shell_session_spec.rb
@@ -50,6 +50,7 @@ end
 describe Shell::ClientSession do
   before do
     Chef::Config[:shell_config] = { :override_runlist => [Chef::RunList::RunListItem.new('shell::override')] }
+    @chef_rest = double("Chef::REST")
     @session = Shell::ClientSession.instance
     @node = Chef::Node.build("foo")
     @session.node = @node
@@ -66,6 +67,7 @@ describe Shell::ClientSession do
     @expansion = Chef::RunList::RunListExpansion.new(@node.chef_environment, [])
 
     @node.run_list.should_receive(:expand).with(@node.chef_environment).and_return(@expansion)
+    Chef::REST.should_receive(:new).with(Chef::Config[:chef_server_url]).and_return(@chef_rest)
     @session.rebuild_context
   end