Fixing #77. As I feared, this was a pretty complicated fix; I had to add a lot of infrastructure to both ParsedFile and Config. All config files now have a timer created for them, and by default they check for file changes every 15 seconds. If there is a change, they get rid of values set by the file (but not set on the cli) and set the new values, then the re-use all of the sections, so that any changed directories or whatever get recreated.

This is still not a 100% solution, since things like open files could still be messed with, but I think this is about as close as we are going to get.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1420 980ebf18-57e1-0310-9a29-db15c13687c0

3 files changed, 181 insertions, 26 deletions

diff --git a/lib/puppet/config.rb b/lib/puppet/config.rb
index f70201cee..c697545ce 100644
--- a/lib/puppet/config.rb
+++ b/lib/puppet/config.rb
@@ -9,10 +9,14 @@ class Config
 
     @@sync = Sync.new
 
+    attr_reader :file, :timer
 
     # Retrieve a config value
     def [](param)
         param = symbolize(param)
+
+        # Yay, recursion.
+        self.reparse() unless param == :filetimeout
         if @config.include?(param)
             if @config[param]
                 val = @config[param].value
@@ -25,14 +29,19 @@ class Config
 
     # Set a config value.  This doesn't set the defaults, it sets the value itself.
     def []=(param, value)
-        param = symbolize(param)
-        unless @config.include?(param)
-            raise Puppet::Error, "Unknown configuration parameter %s" % param.inspect
-        end
-        unless @order.include?(param)
-            @order << param
+        @@sync.synchronize do # yay, thread-safe
+            param = symbolize(param)
+            unless @config.include?(param)
+                raise Puppet::Error,
+                    "Unknown configuration parameter %s" % param.inspect
+            end
+            unless @order.include?(param)
+                @order << param
+            end
+            @config[param].value = value
         end
-        @config[param].value = value
+
+        return value
     end
 
     # A simplified equality operator.
@@ -91,14 +100,19 @@ class Config
         end
     end
 
-    # Remove all set values.
+    # Remove all set values, potentially skipping cli values.
     def clear(exceptcli = false)
         @config.each { |name, obj|
             unless exceptcli and obj.setbycli
                 obj.clear
             end
         }
-        @used = []
+
+        # Don't clear the 'used' in this case, since it's a config file reparse,
+        # and we want to retain this info.
+        unless exceptcli
+            @used = []
+        end
     end
 
     # This is mostly just used for testing.
@@ -140,7 +154,7 @@ class Config
 
     # Handle a command-line argument.
     def handlearg(opt, value = nil)
-        value = mungearg(value)
+        value = mungearg(value) if value
         str = opt.sub(/^--/,'')
         bool = true
         newstr = str.sub(/^no-/, '')
@@ -201,7 +215,7 @@ class Config
             when /^true$/i: true
             when /^\d+$/i: Integer(value)
             else
-                value
+                value.gsub(/^["']|["']$/,'')
         end
     end
 
@@ -219,21 +233,30 @@ class Config
     def parse(file)
         text = nil
 
+        if file.is_a? Puppet::ParsedFile
+            @file = file
+        else
+            @file = Puppet::ParsedFile.new(file)
+        end
+
+        # Create a timer so that this.
+        settimer()
+
         begin
-            text = File.read(file)
+            text = File.read(@file.file)
         rescue Errno::ENOENT
             raise Puppet::Error, "No such file %s" % file
         rescue Errno::EACCES
             raise Puppet::Error, "Permission denied to file %s" % file
         end
 
-        # Store it for later, in a way that we can test and such.
-        @file = Puppet::ParsedFile.new(file)
-
         @values = Hash.new { |names, name|
             names[name] = {}
         }
 
+        # Get rid of the values set by the file, keeping cli values.
+        self.clear(true)
+
         section = "puppet"
         metas = %w{owner group mode}
         values = Hash.new { |hash, key| hash[key] = {} }
@@ -321,6 +344,25 @@ class Config
         }
     end
 
+    # Reparse our config file, if necessary.
+    def reparse
+        if defined? @file and @file.changed?
+            Puppet.notice "Reparsing %s" % @file.file
+            parse(@file)
+            reuse()
+        end
+    end
+
+    def reuse
+        return unless defined? @used
+        @@sync.synchronize do # yay, thread-safe
+            @used.each do |section|
+                @used.delete(section)
+                self.use(section)
+            end
+        end
+    end
+
     # Get a list of objects per section
     def sectionlist
         sectionlist = []
@@ -418,6 +460,19 @@ class Config
         }
     end
 
+    # Create a timer to check whether the file should be reparsed.
+    def settimer
+        if Puppet[:filetimeout] > 0
+            @timer = Puppet.newtimer(
+                :interval => Puppet[:filetimeout],
+                :tolerance => 1,
+                :start? => true
+            ) do
+                self.reparse()
+            end
+        end
+    end
+
     def symbolize(param)
         case param
         when String: return param.intern
@@ -497,16 +552,6 @@ Generated on #{Time.now}.
         return manifest
     end
 
-    def reuse
-        return unless defined? @used
-        @@sync.synchronize do # yay, thread-safe
-            @used.each do |section|
-                @used.delete(section)
-                self.use(section)
-            end
-        end
-    end
-
     # Create the necessary objects to use a section.  This is idempotent;
     # you can 'use' a section as many times as you want.
     def use(*sections)
diff --git a/lib/puppet/parsedfile.rb b/lib/puppet/parsedfile.rb
index c5168b2b1..f256ca522 100755
--- a/lib/puppet/parsedfile.rb
+++ b/lib/puppet/parsedfile.rb
@@ -19,7 +19,7 @@ module Puppet
         # be reparsed
         def changed?
             # Don't actually stat the file more often than filetimeout.
-            if Time.now - @statted > Puppet[:filetimeout]
+            if Time.now - @statted >= Puppet[:filetimeout]
                 tmp = stamp()
 
                 if tmp == @tstamp
diff --git a/test/other/config.rb b/test/other/config.rb
index 2b3c985a4..33dad6892 100755
--- a/test/other/config.rb
+++ b/test/other/config.rb
@@ -695,7 +695,117 @@ inttest = 27
         end
     end
 
+    # Make sure we correctly reparse our config files but don't lose CLI values.
     def test_reparse
+        Puppet[:filetimeout] = 0
+
+        config = mkconfig()
+        config.setdefaults(:mysection, :default => ["default", "yay"])
+        config.setdefaults(:mysection, :clichange => ["clichange", "yay"])
+        config.setdefaults(:mysection, :filechange => ["filechange", "yay"])
+
+        file = tempfile()
+        # Set one parameter in the file
+        File.open(file, "w") { |f|
+            f.puts %{[mysection]\nfilechange = filevalue}
+        }
+        assert_nothing_raised {
+            config.parse(file)
+        }
+
+        # Set another "from the cli"
+        assert_nothing_raised {
+            config.handlearg("clichange", "clivalue")
+        }
+
+        # And leave the other unset
+        assert_equal("default", config[:default])
+        assert_equal("filevalue", config[:filechange])
+        assert_equal("clivalue", config[:clichange])
+
+        # Now rewrite the file
+        File.open(file, "w") { |f|
+            f.puts %{[mysection]\nfilechange = newvalue}
+        }
+
+        cfile = config.file
+        cfile.send("tstamp=".intern, Time.now - 50)
+
+        # And check all of the values
+        assert_equal("default", config[:default])
+        assert_equal("clivalue", config[:clichange])
+        assert_equal("newvalue", config[:filechange])
+    end
+
+    def test_parse_removes_quotes
+        config = mkconfig()
+        config.setdefaults(:mysection, :singleq => ["single", "yay"])
+        config.setdefaults(:mysection, :doubleq => ["double", "yay"])
+        config.setdefaults(:mysection, :none => ["noquote", "yay"])
+        config.setdefaults(:mysection, :middle => ["midquote", "yay"])
+
+        file = tempfile()
+        # Set one parameter in the file
+        File.open(file, "w") { |f|
+            f.puts %{[mysection]\n
+    singleq = 'one'
+    doubleq = "one"
+    none = one
+    middle = mid"quote
+}
+        }
+
+        assert_nothing_raised {
+            config.parse(file)
+        }
+
+        %w{singleq doubleq none}.each do |p|
+            assert_equal("one", config[p], "%s did not match" % p)
+        end
+        assert_equal('mid"quote', config["middle"], "middle did not match")
+    end
+
+    def test_timer
+        Puppet[:filetimeout] = 0.1
+        origpath = tempfile()
+        config = mkconfig()
+        config.setdefaults(:mysection, :paramdir => [tempfile(), "yay"])
+
+        file = tempfile()
+        # Set one parameter in the file
+        File.open(file, "w") { |f|
+            f.puts %{[mysection]\n
+    paramdir = #{origpath}
+}
+        }
+
+        assert_nothing_raised {
+            config.parse(file)
+            config.use(:mysection)
+        }
+
+        assert(FileTest.directory?(origpath), "dir did not get created")
+
+        # Now start the timer
+        assert_nothing_raised {
+            EventLoop.current.monitor_timer config.timer
+        }
+
+        newpath = tempfile()
+
+        File.open(file, "w") { |f|
+            f.puts %{[mysection]\n
+    paramdir = #{newpath}
+}
+        }
+        config.file.send("tstamp=".intern, Time.now - 50)
+        sleep 1
+
+        assert_equal(newpath, config["paramdir"],
+                    "File did not get reparsed from timer")
+        assert(FileTest.directory?(newpath), "new dir did not get created")
+
+
     end
 end