Refactoring how the Facter integration works

I moved all of the extra Fact modifications into the Facts
class, and then moved the calls of those new methods
into the Facter terminus.

Signed-off-by: Luke Kanies <luke@madstop.com>

6 files changed, 148 insertions, 94 deletions

diff --git a/lib/puppet/agent/fact_handler.rb b/lib/puppet/agent/fact_handler.rb
index 4c9280bfc..266ae1815 100644
--- a/lib/puppet/agent/fact_handler.rb
+++ b/lib/puppet/agent/fact_handler.rb
@@ -1,3 +1,5 @@
+require 'puppet/indirector/facts/facter'
+
 # Break out the code related to facts.  This module is
 # just included into the agent, but having it here makes it
 # easier to test.
@@ -24,31 +26,6 @@ module Puppet::Agent::FactHandler
         Puppet::Agent::Downloader.new("fact", Puppet[:factsource], Puppet[:factdest], Puppet[:factsignore]).evaluate
     end
 
-    def load_fact_plugins
-        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = Puppet[:factpath].split(":").each do |dir|
-            load_facts_in_dir(dir)
-        end
-    end
-
-    def load_facts_in_dir(dir)
-        return unless FileTest.directory?(dir)
-
-        Dir.chdir(dir) do
-            Dir.glob("*.rb").each do |file|
-                fqfile = ::File.join(dir, file)
-                begin
-                    Puppet.info "Loading facts in %s" % [::File.basename(file.sub(".rb",''))]
-                    Timeout::timeout(Puppet::Agent.timeout) do
-                        load file
-                    end
-                rescue => detail
-                    Puppet.warning "Could not load fact file %s: %s" % [fqfile, detail]
-                end
-            end
-        end
-    end
-
     # Clear out all of the loaded facts and reload them from disk.
     # NOTE: This is clumsy and shouldn't be required for later (1.5.x) versions
     # of Facter.
@@ -66,6 +43,6 @@ module Puppet::Agent::FactHandler
 
         # This loads all existing facts and any new ones.  We have to remove and
         # reload because there's no way to unload specific facts.
-        load_fact_plugins()
+        Puppet::Node::Facts::Facter.load_fact_plugins()
     end
 end
diff --git a/lib/puppet/indirector/facts/facter.rb b/lib/puppet/indirector/facts/facter.rb
index bc4a0e840..6376b71ca 100644
--- a/lib/puppet/indirector/facts/facter.rb
+++ b/lib/puppet/indirector/facts/facter.rb
@@ -6,27 +6,29 @@ class Puppet::Node::Facts::Facter < Puppet::Indirector::Code
         between Puppet and Facter.  It's only `somewhat` abstract because it always
         returns the local host's facts, regardless of what you attempt to find."
 
-    def self.loaddir(dir, type)
-        return unless FileTest.directory?(dir)
 
-        Dir.entries(dir).find_all { |e| e =~ /\.rb$/ }.each do |file|
-            fqfile = ::File.join(dir, file)
-            begin
-                Puppet.info "Loading %s %s" % 
-                    [type, ::File.basename(file.sub(".rb",''))]
-                Timeout::timeout(self.timeout) do
-                    load fqfile
-                end
-            rescue => detail
-                Puppet.warning "Could not load %s %s: %s" % [type, fqfile, detail]
-            end
+    def self.load_fact_plugins
+        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
+        x = Puppet[:factpath].split(":").each do |dir|
+            load_facts_in_dir(dir)
         end
     end
 
-    def self.loadfacts
-        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = Puppet[:factpath].split(":").each do |dir|
-            loaddir(dir, "fact")
+    def self.load_facts_in_dir(dir)
+        return unless FileTest.directory?(dir)
+
+        Dir.chdir(dir) do
+            Dir.glob("*.rb").each do |file|
+                fqfile = ::File.join(dir, file)
+                begin
+                    Puppet.info "Loading facts in %s" % [::File.basename(file.sub(".rb",''))]
+                    Timeout::timeout(self.timeout) do
+                        load file
+                    end
+                rescue => detail
+                    Puppet.warning "Could not load fact file %s: %s" % [fqfile, detail]
+                end
+            end
         end
     end
 
@@ -49,7 +51,7 @@ class Puppet::Node::Facts::Facter < Puppet::Indirector::Code
 
     def initialize(*args)
         super
-        self.class.loadfacts
+        self.class.load_fact_plugins
     end
 
     def destroy(facts)
@@ -59,18 +61,15 @@ class Puppet::Node::Facts::Facter < Puppet::Indirector::Code
     # Look a host's facts up in Facter.
     def find(request)
         result = Puppet::Node::Facts.new(request.key, Facter.to_hash)
-        add_local_facts(result)
+
+        result.add_local_facts
+        result.stringify
+        result.downcase_if_necessary
+
         result
     end
 
     def save(facts)
         raise Puppet::DevError, "You cannot save facts to the code store; it is only used for getting facts from Facter"
     end
-
-    private
-
-    def add_local_facts(facts)
-        facts.values["clientversion"] = Puppet.version.to_s
-        facts.values["environment"] ||= Puppet.settings[:environment]
-    end
 end
diff --git a/lib/puppet/node/facts.rb b/lib/puppet/node/facts.rb
index 8ee90b4ac..dca435c7d 100755
--- a/lib/puppet/node/facts.rb
+++ b/lib/puppet/node/facts.rb
@@ -21,6 +21,11 @@ class Puppet::Node::Facts
 
     attr_accessor :name, :values
 
+    def add_local_facts
+        values["clientversion"] = Puppet.version.to_s
+        values["environment"] ||= Puppet.settings[:environment]
+    end
+
     def initialize(name, values = {})
         @name = name
         @values = values
@@ -28,6 +33,22 @@ class Puppet::Node::Facts
         add_internal
     end
 
+    def downcase_if_necessary
+        return unless Puppet.settings[:downcasefacts]
+
+        Puppet.warning "DEPRECATION NOTICE: Fact downcasing is deprecated; please disable (20080122)"
+        values.each do |fact, value|
+            values[fact] = value.downcase if value.is_a?(String)
+        end
+    end
+
+    # Convert all fact values into strings.
+    def stringify
+        values.each do |fact, value|
+            values[fact] = value.to_s
+        end
+    end
+
     private
 
     # Add internal data to the facts for storage.
diff --git a/spec/unit/agent/fact_handler.rb b/spec/unit/agent/fact_handler.rb
index b58f55ebc..9622b0649 100755
--- a/spec/unit/agent/fact_handler.rb
+++ b/spec/unit/agent/fact_handler.rb
@@ -82,36 +82,9 @@ describe Puppet::Agent::FactHandler do
             @facthandler.reload_facter
         end
 
-        it "should load all Puppet Fact plugins" do
-            @facthandler.expects(:load_fact_plugins)
+        it "should use the Facter terminus load all Puppet Fact plugins" do
+            Puppet::Node::Facts::Facter.expects(:load_fact_plugins)
             @facthandler.reload_facter
         end
     end
-
-    it "should load each directory in the Fact path when loading fact plugins" do
-        Puppet.settings.expects(:value).with(:factpath).returns("one%stwo" % File::PATH_SEPARATOR)
-
-        @facthandler.expects(:load_facts_in_dir).with("one")
-        @facthandler.expects(:load_facts_in_dir).with("two")
-
-        @facthandler.load_fact_plugins
-    end
-
-    it "should skip files when asked to load a directory" do
-        FileTest.expects(:directory?).with("myfile").returns false
-
-        @facthandler.load_facts_in_dir("myfile")
-    end
-
-    it "should load each ruby file when asked to load a directory" do
-        FileTest.expects(:directory?).with("mydir").returns true
-        Dir.expects(:chdir).with("mydir").yields
-
-        Dir.expects(:glob).with("*.rb").returns %w{a.rb b.rb}
-
-        @facthandler.expects(:load).with("a.rb")
-        @facthandler.expects(:load).with("b.rb")
-
-        @facthandler.load_facts_in_dir("mydir")
-    end
 end
diff --git a/spec/unit/indirector/facts/facter.rb b/spec/unit/indirector/facts/facter.rb
index 10b7af398..c0b9dce36 100755
--- a/spec/unit/indirector/facts/facter.rb
+++ b/spec/unit/indirector/facts/facter.rb
@@ -26,7 +26,7 @@ describe Puppet::Node::Facts::Facter do
     end
 
     it "should load facts on initialization" do
-        Puppet::Node::Facts::Facter.expects(:loadfacts)
+        Puppet::Node::Facts::Facter.expects(:load_fact_plugins)
         Puppet::Node::Facts::Facter.new
     end
 end
@@ -40,7 +40,6 @@ describe Puppet::Node::Facts::Facter do
     end
 
     describe Puppet::Node::Facts::Facter, " when finding facts" do
-
         it "should return a Facts instance" do
             @facter.find(@request).should be_instance_of(Puppet::Node::Facts)
         end
@@ -55,21 +54,28 @@ describe Puppet::Node::Facts::Facter do
             facts.values["one"].should == "two"
         end
 
-        it "should add the Puppet version as a 'clientversion' fact" do
-            Facter.expects(:to_hash).returns("one" => "two")
-            @facter.find(@request).values["clientversion"].should == Puppet.version.to_s
+        it "should add local facts" do
+            facts = Puppet::Node::Facts.new("foo")
+            Puppet::Node::Facts.expects(:new).returns facts
+            facts.expects(:add_local_facts)
+
+            @facter.find(@request)
         end
 
-        it "should add the current environment as a fact if one is not set" do
-            Facter.expects(:to_hash).returns("one" => "two")
+        it "should convert all facts into strings" do
+            facts = Puppet::Node::Facts.new("foo")
+            Puppet::Node::Facts.expects(:new).returns facts
+            facts.expects(:stringify)
 
-            @facter.find(@request).values["environment"].should == Puppet[:environment]
+            @facter.find(@request)
         end
 
-        it "should not replace any existing environment fact" do
-            Facter.expects(:to_hash).returns("one" => "two", "environment" => "foo")
+        it "should call the downcase hook" do
+            facts = Puppet::Node::Facts.new("foo")
+            Puppet::Node::Facts.expects(:new).returns facts
+            facts.expects(:downcase_if_necessary)
 
-            @facter.find(@request).values["environment"].should == "foo"
+            @facter.find(@request)
         end
     end
 
@@ -87,7 +93,30 @@ describe Puppet::Node::Facts::Facter do
         end
     end
 
-    describe Puppet::Node::Facts::Facter, " when loading facts from the factpath" do
-        it "should load every fact in each factpath directory"
+    it "should load each directory in the Fact path when loading fact plugins" do
+        Puppet.settings.expects(:value).with(:factpath).returns("one%stwo" % File::PATH_SEPARATOR)
+
+        Puppet::Node::Facts::Facter.expects(:load_facts_in_dir).with("one")
+        Puppet::Node::Facts::Facter.expects(:load_facts_in_dir).with("two")
+
+        Puppet::Node::Facts::Facter.load_fact_plugins
+    end
+
+    it "should skip files when asked to load a directory" do
+        FileTest.expects(:directory?).with("myfile").returns false
+
+        Puppet::Node::Facts::Facter.load_facts_in_dir("myfile")
+    end
+
+    it "should load each ruby file when asked to load a directory" do
+        FileTest.expects(:directory?).with("mydir").returns true
+        Dir.expects(:chdir).with("mydir").yields
+
+        Dir.expects(:glob).with("*.rb").returns %w{a.rb b.rb}
+
+        Puppet::Node::Facts::Facter.expects(:load).with("a.rb")
+        Puppet::Node::Facts::Facter.expects(:load).with("b.rb")
+
+        Puppet::Node::Facts::Facter.load_facts_in_dir("mydir")
     end
 end
diff --git a/spec/unit/node/facts.rb b/spec/unit/node/facts.rb
index 118713b17..a557066b8 100755
--- a/spec/unit/node/facts.rb
+++ b/spec/unit/node/facts.rb
@@ -5,6 +5,61 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/node/facts'
 
 describe Puppet::Node::Facts, "when indirecting" do
+    before do
+        @facts = Puppet::Node::Facts.new("me")
+    end
+
+    it "should be able to convert all fact values to strings" do
+        @facts.values["one"] = 1
+        @facts.stringify
+        @facts.values["one"].should == "1"
+    end
+
+    it "should add the Puppet version as a 'clientversion' fact when adding local facts" do
+        @facts.add_local_facts
+        @facts.values["clientversion"].should == Puppet.version.to_s
+    end
+
+    it "should add the current environment as a fact if one is not set when adding local facts" do
+        @facts.add_local_facts
+        @facts.values["environment"].should == Puppet[:environment]
+    end
+
+    it "should not replace any existing environment fact when adding local facts" do
+        @facts.values["environment"] = "foo"
+        @facts.add_local_facts
+        @facts.values["environment"].should == "foo"
+    end
+
+    it "should be able to downcase fact values" do
+        Puppet.settings.stubs(:value).returns "eh"
+        Puppet.settings.expects(:value).with(:downcasefacts).returns true
+
+        @facts.values["one"] = "Two"
+        
+        @facts.downcase_if_necessary
+        @facts.values["one"].should == "two"
+    end
+
+    it "should only try to downcase strings" do
+        Puppet.settings.stubs(:value).returns "eh"
+        Puppet.settings.expects(:value).with(:downcasefacts).returns true
+
+        @facts.values["now"] = Time.now
+
+        @facts.downcase_if_necessary
+        @facts.values["now"].should be_instance_of(Time)
+    end
+
+    it "should not downcase facts if not configured to do so" do
+        Puppet.settings.stubs(:value).returns "eh"
+        Puppet.settings.expects(:value).with(:downcasefacts).returns false
+
+        @facts.values["one"] = "Two"
+        @facts.downcase_if_necessary
+        @facts.values["one"].should == "Two"
+    end
+
     describe "when indirecting" do
         before do
             @indirection = stub 'indirection', :request => mock('request'), :name => :facts