(#5073) Download plugins even if you're filtering on tags

When we eval a resource in transaction.rb it was being skipped when
filtering on tags and downloading the plugins.  There's a lot of
complicated conditions for whether to skip a resource, but this is a
condensed version of the path that was causing plugins not to be
downloaded.

  skip?
    missing_tags?
      !ignore_tags?
        !host_config

The Puppet::Configurer::Downloader creates separate catalogs and applies them
to get custom facts and plugins, so should be setting host_config to false.
Puppet::Util::Settings also sets host_config to false when you call use on
settings, while normal catalog application defaults to true.

Thanks to Stefan Schulte <stefan.schulte@taunusstein.net> for suggesting
the implementation fix.

3 files changed, 31 insertions, 11 deletions

diff --git a/lib/puppet/configurer/downloader.rb b/lib/puppet/configurer/downloader.rb
index 1b587ed4b..b3696201a 100644
--- a/lib/puppet/configurer/downloader.rb
+++ b/lib/puppet/configurer/downloader.rb
@@ -50,6 +50,7 @@ class Puppet::Configurer::Downloader
 
   def catalog
     catalog = Puppet::Resource::Catalog.new
+    catalog.host_config = false
     catalog.add_resource(file)
     catalog
   end
diff --git a/lib/puppet/configurer/plugin_handler.rb b/lib/puppet/configurer/plugin_handler.rb
index cfc6b5a0b..ae088f26f 100644
--- a/lib/puppet/configurer/plugin_handler.rb
+++ b/lib/puppet/configurer/plugin_handler.rb
@@ -9,7 +9,14 @@ module Puppet::Configurer::PluginHandler
   # Retrieve facts from the central server.
   def download_plugins
     return nil unless download_plugins?
-    Puppet::Configurer::Downloader.new("plugin", Puppet[:plugindest], Puppet[:pluginsource], Puppet[:pluginsignore]).evaluate.each { |file| load_plugin(file) }
+    plugin_downloader = Puppet::Configurer::Downloader.new(
+      "plugin",
+      Puppet[:plugindest],
+      Puppet[:pluginsource],
+      Puppet[:pluginsignore]
+    )
+
+    plugin_downloader.evaluate.each { |file| load_plugin(file) }
   end
 
   def load_plugin(file)
diff --git a/spec/unit/configurer/downloader_spec.rb b/spec/unit/configurer/downloader_spec.rb
index c57f39fb5..4080263e7 100755
--- a/spec/unit/configurer/downloader_spec.rb
+++ b/spec/unit/configurer/downloader_spec.rb
@@ -5,6 +5,8 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/configurer/downloader'
 
 describe Puppet::Configurer::Downloader do
+  require 'puppet_spec/files'
+  include PuppetSpec::Files
   it "should require a name" do
     lambda { Puppet::Configurer::Downloader.new }.should raise_error(ArgumentError)
   end
@@ -96,25 +98,35 @@ describe Puppet::Configurer::Downloader do
 
   describe "when creating the catalog to do the downloading" do
     before do
-      @dler = Puppet::Configurer::Downloader.new("foo", "path", "source")
+      @dler = Puppet::Configurer::Downloader.new("foo", "/download/path", "source")
     end
 
     it "should create a catalog and add the file to it" do
-      file = mock 'file'
-      catalog = mock 'catalog'
-
-      @dler.expects(:file).returns file
-
-      Puppet::Resource::Catalog.expects(:new).returns catalog
-      catalog.expects(:add_resource).with(file)
+      catalog = @dler.catalog
+      catalog.resources.size.should == 1
+      catalog.resources.first.class.should == Puppet::Type::File
+      catalog.resources.first.name.should == "/download/path"
+    end
 
-      @dler.catalog.should equal(catalog)
+    it "should specify that it is not managing a host catalog" do
+      @dler.catalog.host_config.should == false
     end
+
   end
 
   describe "when downloading" do
     before do
-      @dler = Puppet::Configurer::Downloader.new("foo", "path", "source")
+      @dl_name = tmpfile("downloadpath")
+      source_name = tmpfile("source")
+      File.open(source_name, 'w') {|f| f.write('hola mundo') }
+      @dler = Puppet::Configurer::Downloader.new("foo", @dl_name, source_name)
+    end
+
+    it "should not skip downloaded resources when filtering on tags" do
+      Puppet[:tags] = 'maytag'
+      @dler.evaluate
+
+      File.exists?(@dl_name).should be_true
     end
 
     it "should log that it is downloading" do