Make FileVendor configuration specific to the two implementations

danielsdeleo · danielsdeleo · commit 3b2b4de8e7f0 · 2014-07-29T17:14:24.000-07:00
FileVendor previously was configured by storing a closure/anonymous
function as a class instance variable. This had the following downsides:
* The API was too general, which caused a lot of code repetition
* The block was lazily evaluated, which hid errors and made testing more
  difficult
* The closures captured references to classes with references to large
  data structures, which complicates GC.

Since we've only ever had the same two implementations of FileVendor, we
can encapsulate configuration of the FileVendor factory by wrapping each
configuration option in a method. As a side benefit, arguments to these
methods will be eagerly evaluated, which makes it easier to detect
errors.
diff --git a/lib/chef/cookbook/file_system_file_vendor.rb b/lib/chef/cookbook/file_system_file_vendor.rb
@@ -31,6 +31,9 @@ class Cookbook
     # 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
@@ -24,15 +24,39 @@ class Cookbook
     # 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
@@ -25,6 +25,9 @@ class Cookbook
     # 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
@@ -184,7 +184,7 @@ def cookbooks_to_upload
 
       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
@@ -56,13 +56,13 @@ def initialize(node_name, ohai_data, json_attribs, override_runlist, events)
 
       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
@@ -154,10 +154,7 @@ def build_node
       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
@@ -169,7 +169,7 @@ def definitions
 
     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 @@ def save_node
 
     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
@@ -40,7 +40,7 @@ def create_resource
     # 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
@@ -135,9 +135,7 @@ def apt_data_dir
       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
@@ -26,7 +26,7 @@
 
   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
@@ -37,7 +37,7 @@ def binread(file)
 
   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
@@ -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
@@ -76,7 +76,7 @@
   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
@@ -339,7 +339,7 @@
   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
@@ -35,7 +35,7 @@ class Chef::CFCCheck
     @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
@@ -36,7 +36,7 @@
 
   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
@@ -50,6 +50,7 @@ def loading_complete
 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 @@ def loading_complete
     @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
 
