(#2128) Get facts before retrieving catalog

Retrieving a catalog and getting the facts to submit with the catalog request
are distinct operations, and should be done separately. This is also to prepare
for adding the ability to determine the node name based on a fact, in which
case the node name needs to be determined before it is used for either the
catalog or the report.

Paired-With: Jacob Helwig <jacob@puppetlabs.com>

2 files changed, 22 insertions, 23 deletions

diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index d0251de82..596d2dc53 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -84,16 +84,8 @@ class Puppet::Configurer
   end
 
   # Get the remote catalog, yo.  Returns nil if no catalog can be found.
-  def retrieve_catalog
-    if Puppet::Resource::Catalog.indirection.terminus_class == :rest
-      # This is a bit complicated.  We need the serialized and escaped facts,
-      # and we need to know which format they're encoded in.  Thus, we
-      # get a hash with both of these pieces of information.
-      fact_options = facts_for_uploading
-    else
-      fact_options = {}
-    end
-
+  def retrieve_catalog(fact_options)
+    fact_options ||= {}
     # First try it with no cache, then with the cache.
     unless (Puppet[:use_cached_catalog] and result = retrieve_catalog_from_cache(fact_options)) or result = retrieve_new_catalog(fact_options)
       if ! Puppet[:usecacheonfailure]
@@ -130,13 +122,20 @@ class Puppet::Configurer
       Puppet.err "Failed to prepare catalog: #{detail}"
     end
 
+    if Puppet::Resource::Catalog.indirection.terminus_class == :rest
+      # This is a bit complicated.  We need the serialized and escaped facts,
+      # and we need to know which format they're encoded in.  Thus, we
+      # get a hash with both of these pieces of information.
+      fact_options = facts_for_uploading
+    end
+
     options[:report] ||= Puppet::Transaction::Report.new("apply")
     report = options[:report]
     Puppet::Util::Log.newdestination(report)
 
     if catalog = options[:catalog]
       options.delete(:catalog)
-    elsif ! catalog = retrieve_catalog
+    elsif ! catalog = retrieve_catalog(fact_options)
       Puppet.err "Could not retrieve catalog; skipping run"
       return
     end
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
index 6c4f9b90a..d454b5fce 100755
--- a/spec/unit/configurer_spec.rb
+++ b/spec/unit/configurer_spec.rb
@@ -375,21 +375,21 @@ describe Puppet::Configurer do
         Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns @catalog
         Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.never
 
-        @agent.retrieve_catalog.should == @catalog
+        @agent.retrieve_catalog({}).should == @catalog
       end
 
       it "should compile a new catalog if none is found in the cache" do
         Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns nil
         Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
 
-        @agent.retrieve_catalog.should == @catalog
+        @agent.retrieve_catalog({}).should == @catalog
       end
     end
 
     it "should use the Catalog class to get its catalog" do
       Puppet::Resource::Catalog.expects(:find).returns @catalog
 
-      @agent.retrieve_catalog
+      @agent.retrieve_catalog({})
     end
 
     it "should use its node_name_value to retrieve the catalog" do
@@ -397,13 +397,13 @@ describe Puppet::Configurer do
       Puppet.settings[:node_name_value] = "myhost.domain.com"
       Puppet::Resource::Catalog.expects(:find).with { |name, options| name == "myhost.domain.com" }.returns @catalog
 
-      @agent.retrieve_catalog
+      @agent.retrieve_catalog({})
     end
 
     it "should default to returning a catalog retrieved directly from the server, skipping the cache" do
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
 
-      @agent.retrieve_catalog.should == @catalog
+      @agent.retrieve_catalog({}).should == @catalog
     end
 
     it "should log and return the cached catalog when no catalog can be retrieved from the server" do
@@ -412,21 +412,21 @@ describe Puppet::Configurer do
 
       Puppet.expects(:notice)
 
-      @agent.retrieve_catalog.should == @catalog
+      @agent.retrieve_catalog({}).should == @catalog
     end
 
     it "should not look in the cache for a catalog if one is returned from the server" do
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.never
 
-      @agent.retrieve_catalog.should == @catalog
+      @agent.retrieve_catalog({}).should == @catalog
     end
 
     it "should return the cached catalog when retrieving the remote catalog throws an exception" do
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.raises "eh"
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns @catalog
 
-      @agent.retrieve_catalog.should == @catalog
+      @agent.retrieve_catalog({}).should == @catalog
     end
 
     it "should log and return nil if no catalog can be retrieved from the server and :usecacheonfailure is disabled" do
@@ -436,27 +436,27 @@ describe Puppet::Configurer do
 
       Puppet.expects(:warning)
 
-      @agent.retrieve_catalog.should be_nil
+      @agent.retrieve_catalog({}).should be_nil
     end
 
     it "should return nil if no cached catalog is available and no catalog can be retrieved from the server" do
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns nil
       Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns nil
 
-      @agent.retrieve_catalog.should be_nil
+      @agent.retrieve_catalog({}).should be_nil
     end
 
     it "should convert the catalog before returning" do
       Puppet::Resource::Catalog.stubs(:find).returns @catalog
 
       @agent.expects(:convert_catalog).with { |cat, dur| cat == @catalog }.returns "converted catalog"
-      @agent.retrieve_catalog.should == "converted catalog"
+      @agent.retrieve_catalog({}).should == "converted catalog"
     end
 
     it "should return nil if there is an error while retrieving the catalog" do
       Puppet::Resource::Catalog.expects(:find).at_least_once.raises "eh"
 
-      @agent.retrieve_catalog.should be_nil
+      @agent.retrieve_catalog({}).should be_nil
     end
   end