Merge pull request #2970 from kylog/joshcooper-ticket/master/PUP-2907

ticket/master/pup 2907

Author: Kylo Ginsberg

lib/puppet/configurer.rb

@@ -8,9 +8,9 @@
 class Puppet::Configurer
   require 'puppet/configurer/fact_handler'
   require 'puppet/configurer/plugin_handler'
+  require 'puppet/configurer/downloader_factory'
 
   include Puppet::Configurer::FactHandler
-  include Puppet::Configurer::PluginHandler
 
   # For benchmarking
   include Puppet::Util
@@ -44,13 +44,14 @@ def init_storage
     end
   end
 
-  def initialize
+  def initialize(factory = Puppet::Configurer::DownloaderFactory.new)
     Puppet.settings.use(:main, :ssl, :agent)
 
     @running = false
     @splayed = false
     @environment = Puppet[:environment]
     @transaction_uuid = SecureRandom.uuid
+    @handler = Puppet::Configurer::PluginHandler.new(factory)
   end
 
   # Get the remote catalog, yo.  Returns nil if no catalog can be found.
@@ -295,4 +296,8 @@ def retrieve_new_catalog(query_options)
     Puppet.log_exception(detail, "Could not retrieve catalog from remote server: #{detail}")
     return nil
   end
+
+  def download_plugins(remote_environment_for_plugins)
+    @handler.download_plugins(remote_environment_for_plugins)
+  end
 end

lib/puppet/configurer/downloader_factory.rb

@@ -0,0 +1,34 @@
+require 'puppet/configurer'
+
+# Factory for <tt>Puppet::Configurer::Downloader</tt> objects.
+#
+# Puppet's pluginsync facilities can be used to download modules
+# and external facts, each with a different destination directory
+# and related settings.
+#
+# @api private
+#
+class Puppet::Configurer::DownloaderFactory
+  def create_plugin_downloader(environment)
+    plugin_downloader = Puppet::Configurer::Downloader.new(
+      "plugin",
+      Puppet[:plugindest],
+      Puppet[:pluginsource],
+      Puppet[:pluginsignore],
+      environment
+    )
+  end
+
+  def create_plugin_facts_downloader(environment)
+    source_permissions = Puppet.features.microsoft_windows? ? :ignore : :use
+
+    plugin_fact_downloader = Puppet::Configurer::Downloader.new(
+      "pluginfacts",
+      Puppet[:pluginfactdest],
+      Puppet[:pluginfactsource],
+      Puppet[:pluginsignore],
+      environment,
+      source_permissions
+    )
+  end
+end

lib/puppet/configurer/plugin_handler.rb

@@ -1,26 +1,20 @@
 # Break out the code related to plugins.  This module is
 # just included into the agent, but having it here makes it
 # easier to test.
-module Puppet::Configurer::PluginHandler
+require 'puppet/configurer'
+
+class Puppet::Configurer::PluginHandler
+  def initialize(factory)
+    @factory = factory
+  end
+
   # Retrieve facts from the central server.
   def download_plugins(environment)
-    plugin_downloader = Puppet::Configurer::Downloader.new(
-      "plugin",
-      Puppet[:plugindest],
-      Puppet[:pluginsource],
-      Puppet[:pluginsignore],
-      environment
-    )
+    plugin_downloader = @factory.create_plugin_downloader(environment)
+
     if Puppet.features.external_facts?
-       plugin_fact_downloader = Puppet::Configurer::Downloader.new(
-          "pluginfacts",
-          Puppet[:pluginfactdest],
-          Puppet[:pluginfactsource],
-          Puppet[:pluginsignore],
-          environment,
-          :use
-       )
-       plugin_fact_downloader.evaluate
+      plugin_fact_downloader = @factory.create_plugin_facts_downloader(environment)
+      plugin_fact_downloader.evaluate
     end
 
     plugin_downloader.evaluate

spec/integration/configurer_spec.rb

@@ -6,20 +6,6 @@
 describe Puppet::Configurer do
   include PuppetSpec::Files
 
-  describe "when downloading plugins" do
-    it "should use the :pluginsignore setting, split on whitespace, for ignoring remote files" do
-      Puppet.settings.stubs(:use)
-      resource = Puppet::Type.type(:notify).new :name => "yay"
-      Puppet::Type.type(:file).expects(:new).at_most(2).with do |args|
-        args[:ignore] == Puppet[:pluginsignore].split(/\s+/)
-      end.returns resource
-
-      configurer = Puppet::Configurer.new
-      configurer.stubs(:download_plugins?).returns true
-      configurer.download_plugins(Puppet::Node::Environment.remote(:testing))
-    end
-  end
-
   describe "when running" do
     before(:each) do
       @catalog = Puppet::Resource::Catalog.new("testing", Puppet.lookup(:environments).get(Puppet[:environment]))

spec/unit/configurer/downloader_factory_spec.rb

@@ -0,0 +1,96 @@
+#! /usr/bin/env ruby
+require 'spec_helper'
+require 'puppet/configurer'
+
+describe Puppet::Configurer::DownloaderFactory do
+  let(:factory)     { Puppet::Configurer::DownloaderFactory.new }
+  let(:environment) { Puppet::Node::Environment.create(:myenv, []) }
+
+  let(:plugin_downloader) do
+    factory.create_plugin_downloader(environment)
+  end
+
+  let(:facts_downloader) do
+    factory.create_plugin_facts_downloader(environment)
+  end
+
+  def ignores_source_permissions(downloader)
+    expect(downloader.file[:source_permissions]).to eq(:ignore)
+  end
+
+  def uses_source_permissions(downloader)
+    expect(downloader.file[:source_permissions]).to eq(:use)
+  end
+
+  context "when creating a plugin downloader for modules" do
+    it 'is named "plugin"' do
+      expect(plugin_downloader.name).to eq('plugin')
+    end
+
+    it 'downloads files into Puppet[:plugindest]' do
+      plugindest = File.expand_path("/tmp/pdest")
+      Puppet[:plugindest] = plugindest
+
+      expect(plugin_downloader.file[:path]).to eq(plugindest)
+    end
+
+    it 'downloads files from Puppet[:pluginsource]' do
+      Puppet[:pluginsource] = 'puppet:///myotherplugins'
+
+      expect(plugin_downloader.file[:source]).to eq([Puppet[:pluginsource]])
+    end
+
+    it 'ignores files from Puppet[:pluginsignore]' do
+      Puppet[:pluginsignore] = 'pignore'
+
+      expect(plugin_downloader.file[:ignore]).to eq(['pignore'])
+    end
+
+    it 'splits Puppet[:pluginsignore] on whitespace' do
+      Puppet[:pluginsignore] = ".svn CVS .git"
+
+      expect(plugin_downloader.file[:ignore]).to eq(%w[.svn CVS .git])
+    end
+
+    it "ignores source permissions" do
+      ignores_source_permissions(plugin_downloader)
+    end
+  end
+
+  context "when creating a plugin downloader for external facts" do
+    it 'is named "pluginfacts"' do
+      expect(facts_downloader.name).to eq('pluginfacts')
+    end
+
+    it 'downloads files into Puppet[:pluginfactdest]' do
+      plugindest = File.expand_path("/tmp/pdest")
+      Puppet[:pluginfactdest] = plugindest
+
+      expect(facts_downloader.file[:path]).to eq(plugindest)
+    end
+
+    it 'downloads files from Puppet[:pluginfactsource]' do
+      Puppet[:pluginfactsource] = 'puppet:///myotherfacts'
+
+      expect(facts_downloader.file[:source]).to eq([Puppet[:pluginfactsource]])
+    end
+
+    it 'ignores files from Puppet[:pluginsignore]' do
+      Puppet[:pluginsignore] = 'pignore'
+
+      expect(facts_downloader.file[:ignore]).to eq(['pignore'])
+    end
+
+    context "on POSIX", :if => Puppet.features.posix? do
+      it "uses source permissions" do
+        uses_source_permissions(facts_downloader)
+      end
+    end
+
+    context "on Windows", :if => Puppet.features.microsoft_windows? do
+      it "ignores source permissions during external fact pluginsync" do
+        ignores_source_permissions(facts_downloader)
+      end
+    end
+  end
+end

spec/unit/configurer/plugin_handler_spec.rb

@@ -3,37 +3,37 @@
 require 'puppet/configurer'
 require 'puppet/configurer/plugin_handler'
 
-class PluginHandlerTester
-  include Puppet::Configurer::PluginHandler
-  attr_accessor :environment
-end
-
 describe Puppet::Configurer::PluginHandler do
-  before do
-    @pluginhandler = PluginHandlerTester.new
+  let(:factory)       { Puppet::Configurer::DownloaderFactory.new }
+  let(:pluginhandler) { Puppet::Configurer::PluginHandler.new(factory) }
+  let(:environment)   { Puppet::Node::Environment.create(:myenv, []) }
 
+  before :each do
     # PluginHandler#load_plugin has an extra-strong rescue clause
     # this mock is to make sure that we don't silently ignore errors
     Puppet.expects(:err).never
   end
 
-  it "should use an Agent Downloader, with the name, source, destination, ignore, and environment set correctly, to download plugins when downloading is enabled" do
-    environment = Puppet::Node::Environment.create(:myenv, [])
-    Puppet.features.stubs(:external_facts?).returns(:true)
-    plugindest = File.expand_path("/tmp/pdest")
-    Puppet[:pluginsource] = "psource"
-    Puppet[:plugindest] = plugindest
-    Puppet[:pluginsignore] = "pignore"
-    Puppet[:pluginfactsource] = "psource"
-    Puppet[:pluginfactdest] = plugindest
+  it "downloads plugins and facts" do
+    Puppet.features.stubs(:external_facts?).returns(true)
+
+    plugin_downloader = stub('plugin-downloader', :evaluate => [])
+    facts_downloader = stub('facts-downloader', :evaluate => [])
+
+    factory.expects(:create_plugin_downloader).returns(plugin_downloader)
+    factory.expects(:create_plugin_facts_downloader).returns(facts_downloader)
+
+    pluginhandler.download_plugins(environment)
+  end
+
+  it "skips facts if not enabled" do
+    Puppet.features.stubs(:external_facts?).returns(false)
 
-    downloader = mock 'downloader'
-    Puppet::Configurer::Downloader.expects(:new).with("pluginfacts", plugindest, "psource", "pignore", environment, :use).returns downloader
-    Puppet::Configurer::Downloader.expects(:new).with("plugin", plugindest, "psource", "pignore", environment).returns downloader
+    plugin_downloader = stub('plugin-downloader', :evaluate => [])
 
-    downloader.stubs(:evaluate).returns([])
-    downloader.expects(:evaluate).twice
+    factory.expects(:create_plugin_downloader).returns(plugin_downloader)
+    factory.expects(:create_plugin_facts_downloader).never
 
-    @pluginhandler.download_plugins(environment)
+    pluginhandler.download_plugins(environment)
   end
 end

spec/unit/configurer_spec.rb

@@ -12,10 +12,6 @@
     Puppet[:report] = true
   end
 
-  it "should include the Plugin Handler module" do
-    Puppet::Configurer.ancestors.should be_include(Puppet::Configurer::PluginHandler)
-  end
-
   it "should include the Fact Handler module" do
     Puppet::Configurer.ancestors.should be_include(Puppet::Configurer::FactHandler)
   end