Refactoring how the Settings file is parsed

The goal of this refactor was to use a cached attribute
for the LoadedFile instance we use to monitor whether
the file needs reparsing.  We were getting tests that
affected later tests because they were holding on to
LoadedFile stubs, somehow.

The other main change here is that the Settings#parse
method now knows how to look up its own file path.

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

6 files changed, 169 insertions, 113 deletions

diff --git a/lib/puppet.rb b/lib/puppet.rb
index 2cbf7fb29..4e0266703 100644
--- a/lib/puppet.rb
+++ b/lib/puppet.rb
@@ -118,10 +118,7 @@ module Puppet
 
     # Parse the config file for this process.
     def self.parse_config
-        if Puppet[:config] and File.exists? Puppet[:config]
-            Puppet.debug "Parsing %s" % Puppet[:config]
-            Puppet.settings.parse(Puppet[:config])
-        end
+        Puppet.settings.parse
     end
 
     # XXX this should all be done using puppet objects, not using
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 3f3b2e10e..88ad9d493 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -212,7 +212,7 @@ class Puppet::Application
     def run
         run_preinit
         parse_options
-        Puppet.parse_config if should_parse_config?
+        Puppet.settings.parse if should_parse_config?
         run_setup
         run_command
     end
@@ -299,4 +299,4 @@ class Puppet::Application
         end
     end
 
-end
\ No newline at end of file
+end
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index 27cee8c9f..f84d2f6e2 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -4,10 +4,13 @@ require 'puppet/transportable'
 require 'getoptlong'
 
 require 'puppet/external/event-loop'
+require 'puppet/util/cacher'
+require 'puppet/util/LoadedFile'
 
 # The class for handling configuration files.
 class Puppet::Util::Settings
     include Enumerable
+    include Puppet::Util::Cacher
 
     attr_accessor :file
     attr_reader :timer
@@ -330,7 +333,18 @@ class Puppet::Util::Settings
 
     # Parse the configuration file.  Just provides
     # thread safety.
-    def parse(file)
+    def parse
+        raise "No :config setting defined; cannot parse unknown config file" unless self[:config]
+
+        # Create a timer so that this file will get checked automatically
+        # and reparsed if necessary.
+        set_filetimeout_timer()
+
+        # Retrieve the value now, so that we don't lose it in the 'clear' call.
+        file = self[:config]
+
+        return unless FileTest.exist?(file)
+
         # We have to clear outside of the sync, because it's
         # also using synchronize().
         clear(true)
@@ -422,12 +436,20 @@ class Puppet::Util::Settings
         }
     end
 
+    # Cache this in an easily clearable way, since we were
+    # having trouble cleaning it up after tests.
+    cached_attr(:file) do
+        if path = self[:config] and FileTest.exist?(path)
+            Puppet::Util::LoadedFile.new(path)
+        end
+    end
+
     # Reparse our config file, if necessary.
     def reparse
-        if defined? @file and @file.changed?
-            Puppet.notice "Reparsing %s" % @file.file
+        if file and file.changed?
+            Puppet.notice "Reparsing %s" % file.file
             @sync.synchronize do
-                parse(@file)
+                parse
             end
             reuse()
         end
@@ -506,8 +528,8 @@ class Puppet::Util::Settings
 
     # Create a timer to check whether the file should be reparsed.
     def set_filetimeout_timer
-        return unless timeout = self[:filetimeout] and timeout > 0
-        EventLoop::Timer.new(:interval => timeout, :tolerance => 1, :start? => true) { self.reparse() }
+        return unless timeout = self[:filetimeout] and timeout = Integer(timeout) and timeout > 0
+        timer = EventLoop::Timer.new(:interval => timeout, :tolerance => 1, :start? => true) { self.reparse() }
     end
 
     # Convert the settings we manage into a catalog full of resources that model those settings.
@@ -619,7 +641,7 @@ Generated on #{Time.now}.
         return nil unless @config.include?(param)
 
         # Yay, recursion.
-        self.reparse() unless param == :filetimeout
+        #self.reparse() unless [:config, :filetimeout].include?(param)
 
         # Check the cache first.  It needs to be a per-environment
         # cache so that we don't spread values from one env
@@ -822,10 +844,6 @@ Generated on #{Time.now}.
     def parse_file(file)
         text = read_file(file)
 
-        # Create a timer so that this file will get checked automatically
-        # and reparsed if necessary.
-        set_filetimeout_timer()
-
         result = Hash.new { |names, name|
             names[name] = {}
         }
@@ -881,14 +899,8 @@ Generated on #{Time.now}.
 
     # Read the file in.
     def read_file(file)
-        if file.is_a? Puppet::Util::LoadedFile
-            @file = file
-        else
-            @file = Puppet::Util::LoadedFile.new(file)
-        end
-
         begin
-            return File.read(@file.file)
+            return File.read(file)
         rescue Errno::ENOENT
             raise ArgumentError, "No such file %s" % file
         rescue Errno::EACCES
diff --git a/spec/unit/application.rb b/spec/unit/application.rb
index e1f9362c4..5c3df0999 100755
--- a/spec/unit/application.rb
+++ b/spec/unit/application.rb
@@ -223,7 +223,7 @@ describe Puppet::Application do
             @app.stubs(:run_command)
             @app.should_parse_config
 
-            Puppet.expects(:parse_config)
+            Puppet.settings.expects(:parse)
 
             @app.run
         end
@@ -232,7 +232,7 @@ describe Puppet::Application do
             @app.stubs(:run_command)
             @app.should_not_parse_config
 
-            Puppet.expects(:parse_config).never
+            Puppet.settings.expects(:parse).never
 
             @app.run
         end
@@ -241,7 +241,7 @@ describe Puppet::Application do
             @app.stubs(:run_command)
             @app.stubs(:should_parse_config?).returns(true)
 
-            Puppet.expects(:parse_config)
+            Puppet.settings.expects(:parse)
 
             @app.run
         end
diff --git a/spec/unit/parser/interpreter.rb b/spec/unit/parser/interpreter.rb
index 211f42df0..fe63bbd65 100755
--- a/spec/unit/parser/interpreter.rb
+++ b/spec/unit/parser/interpreter.rb
@@ -35,11 +35,9 @@ describe Puppet::Parser::Interpreter do
             # Set our per-environment values.  We can't just stub :value, because
             # it's called by too much of the rest of the code.
             text = "[env1]\nmanifest = /t/env1.pp\n[env2]\nmanifest = /t/env2.pp"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            Puppet.settings.stubs(:read_file).with(file).returns(text)
-            Puppet.settings.parse(file)
+            FileTest.stubs(:exist?).returns true
+            Puppet.settings.stubs(:read_file).returns(text)
+            Puppet.settings.parse
 
             parser1 = mock 'parser1'
             Puppet::Parser::Parser.expects(:new).with(:environment => :env1).returns(parser1)
diff --git a/spec/unit/util/settings.rb b/spec/unit/util/settings.rb
index f84c467e2..c1d74a88b 100755
--- a/spec/unit/util/settings.rb
+++ b/spec/unit/util/settings.rb
@@ -145,7 +145,8 @@ describe Puppet::Util::Settings do
     describe "when returning values" do
         before do
             @settings = Puppet::Util::Settings.new
-            @settings.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"], :four => ["$two $three FOUR", "d"]
+            @settings.setdefaults :section, :config => ["/my/file", "eh"], :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"], :four => ["$two $three FOUR", "d"]
+            FileTest.stubs(:exist?).returns true
         end
 
         it "should provide a mechanism for returning set values" do
@@ -182,11 +183,8 @@ describe Puppet::Util::Settings do
 
         it "should not cache values such that information from one environment is returned for another environment" do
             text = "[env1]\none = oneval\n[env2]\none = twoval\n"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
 
             @settings.value(:one, "env1").should == "oneval"
             @settings.value(:one, "env2").should == "twoval"
@@ -204,8 +202,10 @@ describe Puppet::Util::Settings do
         before do
             @settings = Puppet::Util::Settings.new
             @settings.setdefaults :section,
+                :config => ["/my/file", "a"],
                 :one => ["ONE", "a"],
                 :name => ["myname", "w"]
+            FileTest.stubs(:exist?).returns true
         end
 
         it "should return default values if no values have been set" do
@@ -214,12 +214,9 @@ describe Puppet::Util::Settings do
 
         it "should return values set on the cli before values set in the configuration file" do
             text = "[main]\none = fileval\n"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:parse_file).returns(text)
+            @settings.stubs(:read_file).returns(text)
             @settings.handlearg("--one", "clival")
-            @settings.parse(file)
+            @settings.parse
 
             @settings[:one].should == "clival"
         end
@@ -232,11 +229,8 @@ describe Puppet::Util::Settings do
 
         it "should return values set in the executable-specific section before values set in the main section" do
             text = "[main]\none = mainval\n[myname]\none = nameval\n"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
 
             @settings[:one].should == "nameval"
         end
@@ -244,22 +238,15 @@ describe Puppet::Util::Settings do
         it "should not return values outside of its search path" do
             text = "[other]\none = oval\n"
             file = "/some/file"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
             @settings[:one].should == "ONE"
         end
 
         it "should return values in a specified environment" do
             text = "[env]\none = envval\n"
-            file = "/some/file"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
             @settings.value(:one, "env").should == "envval"
         end
 
@@ -271,12 +258,8 @@ describe Puppet::Util::Settings do
 
         it "should return values in a specified environment before values in the main or name sections" do
             text = "[env]\none = envval\n[main]\none = mainval\n[myname]\none = nameval\n"
-            file = "/some/file"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/whatever")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
             @settings.value(:one, "env").should == "envval"
         end
     end
@@ -284,33 +267,53 @@ describe Puppet::Util::Settings do
     describe "when parsing its configuration" do
         before do
             @settings = Puppet::Util::Settings.new
-            @settings.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+            @file = "/some/file"
+            @settings.setdefaults :section, :config => ["/some/file", "eh"], :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+            FileTest.stubs(:exist?).returns true
         end
 
-        it "should set a timer that triggers reparsing" do
+        it "should use its current ':config' value for the file to parse" do
+            @settings[:config] = "/my/file"
+
+            File.expects(:read).with("/my/file").returns("[main]")
+
+            @settings.parse
+        end
+
+        it "should fail if no configuration setting is defined" do
+            @settings = Puppet::Util::Settings.new
+            lambda { @settings.parse }.should raise_error(RuntimeError)
+        end
+
+        it "should not try to parse non-existent files" do
+            FileTest.expects(:exist?).with("/some/file").returns false
+
+            File.expects(:read).with("/some/file").never
+
+            @settings.parse
+        end
+
+        it "should set a timer that triggers reparsing, even if the file does not exist" do
+            FileTest.expects(:exist?).returns false
             @settings.expects(:set_filetimeout_timer)
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns("[main]")
 
-            @settings.parse(file)
+            @settings.parse
         end
 
         it "should return values set in the configuration file" do
             text = "[main]
             one = fileval
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:one].should == "fileval"
         end
 
         #484 - this should probably be in the regression area
         it "should not throw an exception on unknown parameters" do
             text = "[main]\nnosuchparam = mval\n"
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            lambda { @settings.parse(file) }.should_not raise_error
+            @settings.expects(:read_file).returns(text)
+            lambda { @settings.parse }.should_not raise_error
         end
 
         it "should convert booleans in the configuration file into Ruby booleans" do
@@ -318,9 +321,8 @@ describe Puppet::Util::Settings do
             one = true
             two = false
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:one].should == true
             @settings[:two].should == false
         end
@@ -329,9 +331,8 @@ describe Puppet::Util::Settings do
             text = "[main]
             one = 65
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:one].should == 65
         end
 
@@ -341,9 +342,8 @@ describe Puppet::Util::Settings do
             text = "[main]
             myfile = /other/file {owner = luke, group = luke, mode = 644}
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:myfile].should == "/other/file"
             @settings.metadata(:myfile).should == {:owner => "luke", :group => "luke", :mode => "644"}
         end
@@ -355,8 +355,8 @@ describe Puppet::Util::Settings do
             myfile = /other/file {owner = luke}
             "
             file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:myfile].should == "/other/file"
             @settings.metadata(:myfile).should == {:owner => "luke"}
         end
@@ -368,9 +368,8 @@ describe Puppet::Util::Settings do
             text = "[main]
             mysetting = setval
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             values.should == ["setval"]
         end
 
@@ -383,9 +382,8 @@ describe Puppet::Util::Settings do
             [puppet]
             mysetting = other
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             values.should == ["setval"]
         end
 
@@ -400,9 +398,8 @@ describe Puppet::Util::Settings do
             [yay]
             mysetting = other
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             values.should == ["other"]
         end
 
@@ -414,9 +411,8 @@ describe Puppet::Util::Settings do
             text = "[main]
             mysetting = $base/setval
             "
-            file = "/some/file"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             values.should == ["yay/setval"]
         end
 
@@ -427,7 +423,7 @@ describe Puppet::Util::Settings do
             myarg =
             "
             @settings.stubs(:read_file).returns(text)
-            @settings.parse("/some/file")
+            @settings.parse
             @settings[:myarg].should == ""
         end
     end
@@ -435,7 +431,59 @@ describe Puppet::Util::Settings do
     describe "when reparsing its configuration" do
         before do
             @settings = Puppet::Util::Settings.new
-            @settings.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+            @settings.setdefaults :section, :config => ["/test/file", "a"], :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+            FileTest.stubs(:exist?).returns true
+        end
+
+        it "should use a LoadedFile instance to determine if the file has changed" do
+            file = mock 'file'
+            Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+
+            file.expects(:changed?)
+
+            @settings.stubs(:parse)
+            @settings.reparse
+        end
+
+        it "should not create the LoadedFile instance and should not parse if the file does not exist" do
+            FileTest.expects(:exist?).with("/test/file").returns false
+            Puppet::Util::LoadedFile.expects(:new).never
+
+            @settings.expects(:parse).never
+
+            @settings.reparse
+        end
+
+        it "should not reparse if the file has not changed" do
+            file = mock 'file'
+            Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+
+            file.expects(:changed?).returns false
+
+            @settings.expects(:parse).never
+
+            @settings.reparse
+        end
+
+        it "should reparse if the file has changed" do
+            file = stub 'file', :file => "/test/file"
+            Puppet::Util::LoadedFile.expects(:new).with("/test/file").returns file
+
+            file.expects(:changed?).returns true
+
+            @settings.expects(:parse)
+
+            @settings.reparse
+        end
+
+        it "should use a cached LoadedFile instance" do
+            first = mock 'first'
+            second = mock 'second'
+            Puppet::Util::LoadedFile.expects(:new).times(2).with("/test/file").returns(first).then.returns(second)
+
+            @settings.file.should equal(first)
+            Puppet::Util::Cacher.expire
+            @settings.file.should equal(second)
         end
 
         it "should replace in-memory values with on-file values" do
@@ -453,7 +501,7 @@ describe Puppet::Util::Settings do
             # This is kinda ridiculous - the reason it parses twice is that
             # it goes to parse again when we ask for the value, because the
             # mock always says it should get reparsed.
-            @settings.expects(:read_file).with(file).returns(text).times(2)
+            @settings.stubs(:read_file).returns(text)
             @settings.reparse
             @settings[:one].should == "disk-replace"
         end
@@ -462,10 +510,8 @@ describe Puppet::Util::Settings do
             @settings.handlearg("--one", "clival")
 
             text = "[main]\none = on-disk\n"
-            file = mock 'file'
-            file.stubs(:file).returns("/test/file")
-            @settings.stubs(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.stubs(:read_file).returns(text)
+            @settings.parse
 
             @settings[:one].should == "clival"
         end
@@ -473,17 +519,14 @@ describe Puppet::Util::Settings do
         it "should remove in-memory values that are no longer set in the file" do
             # Init the value
             text = "[main]\none = disk-init\n"
-            file = mock 'file'
-            file.stubs(:changed?).returns(true)
-            file.stubs(:file).returns("/test/file")
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             @settings[:one].should == "disk-init"
 
             # Now replace the value
             text = "[main]\ntwo = disk-replace\n"
-            @settings.expects(:read_file).with(file).returns(text)
-            @settings.parse(file)
+            @settings.expects(:read_file).returns(text)
+            @settings.parse
             #@settings.reparse
 
             # The originally-overridden value should be replaced with the default
@@ -865,6 +908,12 @@ describe Puppet::Util::Settings do
             @settings.set_filetimeout_timer
         end
 
+        it "should always convert the timer interval to an integer" do
+            @settings.expects(:value).with(:filetimeout).returns "10"
+            EventLoop::Timer.expects(:new).with(:interval => 10, :start? => true, :tolerance => 1)
+            @settings.set_filetimeout_timer
+        end
+
         it "should do nothing if the filetimeout setting is not greater than 0" do
             @settings.expects(:value).with(:filetimeout).returns -2
             EventLoop::Timer.expects(:new).never