Making all test suites executable, adding some tests for handling changing files from one type to another, and fixing #304. The problem with #304 was only occurring when backing up to a filebucket (I can only think the example code was wrong)

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

38 files changed, 312 insertions, 167 deletions

diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 3a07acac7..7f2ff798c 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -318,25 +318,12 @@ module Puppet
                             end
                         end
 
-                        require 'fileutils'
-                        FileUtils.rmtree(self[:path])
                         return true
                     when String:
                         newfile = file + backup
                         # Just move it, since it's a directory.
-                        if FileTest.directory?(newfile)
-                            raise Puppet::Error, "Will not replace directory backup; use a filebucket"
-                        elsif FileTest.exists?(newfile)
-                            begin
-                                File.unlink(newfile)
-                            rescue => detail
-                                if Puppet[:trace]
-                                    puts detail.backtrace
-                                end
-                                self.err "Could not remove old backup: %s" %
-                                    detail
-                                return false
-                            end
+                        if FileTest.exists?(newfile)
+                            remove_backup(newfile)
                         end
                         begin
                             bfile = file + backup
@@ -367,13 +354,7 @@ module Puppet
                 when String:
                     newfile = file + backup
                     if FileTest.exists?(newfile)
-                        begin
-                            File.unlink(newfile)
-                        rescue => detail
-                            self.err "Could not remove old backup: %s" %
-                                detail
-                            return false
-                        end
+                        remove_backup(newfile)
                     end
                     begin
                         # FIXME Shouldn't this just use a Puppet object with
@@ -394,6 +375,7 @@ module Puppet
                     self.err "Invalid backup type %s" % backup.inspect
                     return false
                 end
+            when "link": return true
             else
                 self.notice "Cannot backup files of type %s" %
                     File.stat(file).ftype
@@ -440,6 +422,108 @@ module Puppet
 
             @stat = nil
         end
+
+        # Build a recursive map of a link source
+        def linkrecurse(recurse)
+            target = @states[:target].should
+
+            method = :lstat
+            if self[:links] == :follow
+                method = :stat
+            end
+
+            targetstat = nil
+            unless FileTest.exist?(target)
+                #self.info "%s does not exist; not recursing" %
+                #    target
+                return
+            end
+            # Now stat our target
+            targetstat = File.send(method, target)
+            unless targetstat.ftype == "directory"
+                #self.info "%s is not a directory; not recursing" %
+                #    target
+                return
+            end
+
+            # Now that we know our corresponding target is a directory,
+            # change our type
+            self[:ensure] = :directory
+
+            unless FileTest.readable? target
+                self.notice "Cannot manage %s: permission denied" % self.name
+                return
+            end
+
+            children = Dir.entries(target).reject { |d| d =~ /^\.+$/ }
+         
+            #Get rid of ignored children
+            if @parameters.include?(:ignore)
+                children = handleignore(children)
+            end  
+
+            added = []
+            children.each do |file|
+                Dir.chdir(target) do
+                    longname = File.join(target, file)
+
+                    # Files know to create directories when recursion
+                    # is enabled and we're making links
+                    args = {
+                        :recurse => recurse,
+                        :ensure => longname
+                    }
+
+                    if child = self.newchild(file, true, args)
+                        unless @children.include?(child)
+                            self.push child
+                            added.push file
+                        end
+                    end
+                end
+            end
+        end
+
+        # Build up a recursive map of what's around right now
+        def localrecurse(recurse)
+            unless FileTest.exist?(self[:path]) and self.stat.directory?
+                #self.info "%s is not a directory; not recursing" %
+                #    self[:path]
+                return
+            end
+
+            unless FileTest.readable? self[:path]
+                self.notice "Cannot manage %s: permission denied" % self.name
+                return
+            end
+
+            children = Dir.entries(self[:path])
+         
+            #Get rid of ignored children
+            if @parameters.include?(:ignore)
+                children = handleignore(children)
+            end  
+
+            added = []
+            children.each { |file|
+                file = File.basename(file)
+                next if file =~ /^\.\.?$/ # skip . and .. 
+                options = {:recurse => recurse}
+
+                if child = self.newchild(file, true, options)
+                    # Mark any unmanaged files for removal if purge is set.
+                    # Use the array rather than [] because tidy uses this method, too.
+                    if @parameters.include?(:purge) and self[:purge] == :true and child.implicit?
+                        child[:ensure] = :absent
+                    end
+
+                    unless @children.include?(child)
+                        self.push child
+                        added.push file
+                    end
+                end
+            }
+        end
         
         # Create a new file or directory object as a child to the current
         # object.
@@ -624,104 +708,105 @@ module Puppet
             end
         end
 
-        # Build a recursive map of a link source
-        def linkrecurse(recurse)
-            target = @states[:target].should
-
-            method = :lstat
-            if self[:links] == :follow
+        # Remove the old backup.
+        def remove_backup(newfile)
+            if self.class.name == :file and self[:links] != :follow
+                method = :lstat
+            else
                 method = :stat
             end
+            old = File.send(method, newfile).ftype
 
-            targetstat = nil
-            unless FileTest.exist?(target)
-                #self.info "%s does not exist; not recursing" %
-                #    target
-                return
-            end
-            # Now stat our target
-            targetstat = File.send(method, target)
-            unless targetstat.ftype == "directory"
-                #self.info "%s is not a directory; not recursing" %
-                #    target
-                return
+            if old == "directory"
+                raise Puppet::Error,
+                    "Will not remove directory backup %s; use a filebucket" %
+                    newfile
             end
 
-            # Now that we know our corresponding target is a directory,
-            # change our type
-            self[:ensure] = :directory
+            info "Removing old backup of type %s" %
+                File.send(method, newfile).ftype
 
-            unless FileTest.readable? target
-                self.notice "Cannot manage %s: permission denied" % self.name
-                return
+            begin
+                File.unlink(newfile)
+            rescue => detail
+                if Puppet[:trace]
+                    puts detail.backtrace
+                end
+                self.err "Could not remove old backup: %s" %
+                    detail
+                return false
             end
+        end
 
-            children = Dir.entries(target).reject { |d| d =~ /^\.+$/ }
-         
-            #Get rid of ignored children
-            if @parameters.include?(:ignore)
-                children = handleignore(children)
-            end  
+        # Remove any existing data.  This is only used when dealing with
+        # links or directories.
+        def remove_existing(should)
+            return unless s = stat(true)
 
-            added = []
-            children.each do |file|
-                Dir.chdir(target) do
-                    longname = File.join(target, file)
+            unless handlebackup
+                self.fail "Could not back up; will not replace"
+            end
 
-                    # Files know to create directories when recursion
-                    # is enabled and we're making links
-                    args = {
-                        :recurse => recurse,
-                        :ensure => longname
-                    }
+            unless should.to_s == "link"
+                return if s.ftype.to_s == should.to_s 
+            end
 
-                    if child = self.newchild(file, true, args)
-                        unless @children.include?(child)
-                            self.push child
-                            added.push file
-                        end
-                    end
+            case s.ftype
+            when "directory":
+                if self[:force] == :true
+                    debug "Removing existing directory for replacement with %s" %
+                        should
+                    FileUtils.rmtree(self[:path])
+                else
+                    notice "Not replacing directory; use 'force' to override"
                 end
+            when "link", "file":
+                debug "Removing existing %s for replacement with %s" %
+                    [s.ftype, should]
+                File.unlink(self[:path])
+            else
+                self.fail "Could not back up files of type %s" % s.ftype
             end
         end
 
-        # Build up a recursive map of what's around right now
-        def localrecurse(recurse)
-            unless FileTest.exist?(self[:path]) and self.stat.directory?
-                #self.info "%s is not a directory; not recursing" %
-                #    self[:path]
-                return
-            end
+        # a wrapper method to make sure the file exists before doing anything
+        def retrieve
+            if @states.include?(:source)
+                # This probably isn't the best place for it, but we need
+                # to make sure that we have a corresponding checksum state.
+                unless @states.include?(:checksum)
+                    self[:checksum] = "md5"
+                end
 
-            unless FileTest.readable? self[:path]
-                self.notice "Cannot manage %s: permission denied" % self.name
-                return
+                # We have to retrieve the source info before the recursion happens,
+                # although I'm not exactly clear on why.
+                @states[:source].retrieve
             end
 
-            children = Dir.entries(self[:path])
-         
-            #Get rid of ignored children
-            if @parameters.include?(:ignore)
-                children = handleignore(children)
-            end  
+            if @parameters.include?(:recurse)
+                self.recurse
+            end
 
-            added = []
-            children.each { |file|
-                file = File.basename(file)
-                next if file =~ /^\.\.?$/ # skip . and .. 
-                options = {:recurse => recurse}
+            unless stat = self.stat(true)
+                self.debug "File does not exist"
+                @states.each { |name,state|
+                    # We've already retrieved the source, and we don't
+                    # want to overwrite whatever it did.  This is a bit
+                    # of a hack, but oh well, source is definitely special.
+                    next if name == :source
+                    state.is = :absent
+                }
 
-                if child = self.newchild(file, true, options)
-                    # Mark any unmanaged files for removal if purge is set.
-                    # Use the array rather than [] because tidy uses this method, too.
-                    if @parameters.include?(:purge) and self[:purge] == :true and child.implicit?
-                        child[:ensure] = :absent
-                    end
+                return
+            end
 
-                    unless @children.include?(child)
-                        self.push child
-                        added.push file
-                    end
+            states().each { |state|
+                # We don't want to call 'describe()' twice, so only do a local
+                # retrieve on the source.
+                if state.name == :source
+                    state.retrieve(false)
+                else
+                    state.retrieve
                 end
             }
         end
@@ -778,48 +863,6 @@ module Puppet
             }
         end
 
-        # a wrapper method to make sure the file exists before doing anything
-        def retrieve
-            if @states.include?(:source)
-                # This probably isn't the best place for it, but we need
-                # to make sure that we have a corresponding checksum state.
-                unless @states.include?(:checksum)
-                    self[:checksum] = "md5"
-                end
-
-                # We have to retrieve the source info before the recursion happens,
-                # although I'm not exactly clear on why.
-                @states[:source].retrieve
-            end
-
-            if @parameters.include?(:recurse)
-                self.recurse
-            end
-
-            unless stat = self.stat(true)
-                self.debug "File does not exist"
-                @states.each { |name,state|
-                    # We've already retrieved the source, and we don't
-                    # want to overwrite whatever it did.  This is a bit
-                    # of a hack, but oh well, source is definitely special.
-                    next if name == :source
-                    state.is = :absent
-                }
-
-                return
-            end
-
-            states().each { |state|
-                # We don't want to call 'describe()' twice, so only do a local
-                # retrieve on the source.
-                if state.name == :source
-                    state.retrieve(false)
-                else
-                    state.retrieve
-                end
-            }
-        end
-
         # Set the checksum, from another state.  There are multiple states that
         # modify the contents of a file, and they need the ability to make sure
         # that the checksum value is in sync.
@@ -930,16 +973,7 @@ module Puppet
         def write(usetmp = true)
             mode = self.should(:mode)
 
-            #if FileTest.exists?(self[:path])
-            if s = stat(false)
-                # this makes sure we have a copy for posterity
-                @backed = self.handlebackup
-
-                if s.ftype == "link" and self[:links] != :follow
-                    # Remove existing links, since we're writing out a file
-                    File.unlink(self[:path])
-                end
-            end
+            remove_existing(:file)
 
             # The temporary file
             path = nil
diff --git a/lib/puppet/type/pfile/ensure.rb b/lib/puppet/type/pfile/ensure.rb
index 2e48e0165..6f7b15d49 100755
--- a/lib/puppet/type/pfile/ensure.rb
+++ b/lib/puppet/type/pfile/ensure.rb
@@ -154,6 +154,10 @@ module Puppet
         end
 
         def sync
+            unless self.should == :absent
+                @parent.remove_existing(self.should)
+            end
+
             event = super
 
             # There are some cases where all of the work does not get done on
diff --git a/lib/puppet/type/pfile/target.rb b/lib/puppet/type/pfile/target.rb
index a2d174c2e..38151bfe0 100644
--- a/lib/puppet/type/pfile/target.rb
+++ b/lib/puppet/type/pfile/target.rb
@@ -27,23 +27,9 @@ module Puppet
         def mklink
             target = self.should
 
-            if stat = @parent.stat
-                unless @parent.handlebackup
-                    self.fail "Could not back up; will not replace with link"
-                end
+            # Clean up any existing objects.
+            @parent.remove_existing(target)
 
-                case stat.ftype
-                when "directory":
-                    if @parent[:force] == :true
-                        FileUtils.rmtree(@parent[:path])
-                    else
-                        notice "Not replacing directory with link; use 'force' to override"
-                        return :nochange
-                    end
-                else
-                    File.unlink(@parent[:path])
-                end
-            end
             Dir.chdir(File.dirname(@parent[:path])) do
                 Puppet::SUIDManager.asuser(@parent.asuser()) do
                     mode = @parent.should(:mode)
diff --git a/test/client/client.rb b/test/client/client.rb
index 7d02f7acf..7d02f7acf 100644..100755
--- a/test/client/client.rb
+++ b/test/client/client.rb
diff --git a/test/client/master.rb b/test/client/master.rb
index 096918918..096918918 100644..100755
--- a/test/client/master.rb
+++ b/test/client/master.rb
diff --git a/test/client/pelement.rb b/test/client/pelement.rb
index d5f8e824a..d5f8e824a 100644..100755
--- a/test/client/pelement.rb
+++ b/test/client/pelement.rb
diff --git a/test/language/lexer.rb b/test/language/lexer.rb
index 26b85b3b7..26b85b3b7 100644..100755
--- a/test/language/lexer.rb
+++ b/test/language/lexer.rb
diff --git a/test/language/node.rb b/test/language/node.rb
index 251e4c4aa..251e4c4aa 100644..100755
--- a/test/language/node.rb
+++ b/test/language/node.rb
diff --git a/test/language/parser.rb b/test/language/parser.rb
index dbe48616a..dbe48616a 100644..100755
--- a/test/language/parser.rb
+++ b/test/language/parser.rb
diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb
index c8c8b077d..c8c8b077d 100644..100755
--- a/test/lib/puppettest.rb
+++ b/test/lib/puppettest.rb
diff --git a/test/other/autoload.rb b/test/other/autoload.rb
index f561a81a9..f561a81a9 100644..100755
--- a/test/other/autoload.rb
+++ b/test/other/autoload.rb
diff --git a/test/other/inifile.rb b/test/other/inifile.rb
index 113036505..113036505 100644..100755
--- a/test/other/inifile.rb
+++ b/test/other/inifile.rb
diff --git a/test/other/log.rb b/test/other/log.rb
index 36b1e163f..36b1e163f 100644..100755
--- a/test/other/log.rb
+++ b/test/other/log.rb
diff --git a/test/other/metrics.rb b/test/other/metrics.rb
index 380ee30bd..380ee30bd 100644..100755
--- a/test/other/metrics.rb
+++ b/test/other/metrics.rb
diff --git a/test/other/provider.rb b/test/other/provider.rb
index 771c54bf9..771c54bf9 100644..100755
--- a/test/other/provider.rb
+++ b/test/other/provider.rb
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index 82b2bc768..82b2bc768 100644..100755
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
diff --git a/test/providers/nameservice.rb b/test/providers/nameservice.rb
index dafc8dcf0..dafc8dcf0 100644..100755
--- a/test/providers/nameservice.rb
+++ b/test/providers/nameservice.rb
diff --git a/test/providers/package.rb b/test/providers/package.rb
index 64c443c00..64c443c00 100644..100755
--- a/test/providers/package.rb
+++ b/test/providers/package.rb
diff --git a/test/providers/provider.rb b/test/providers/provider.rb
index 28d92e3d6..28d92e3d6 100644..100755
--- a/test/providers/provider.rb
+++ b/test/providers/provider.rb
diff --git a/test/providers/user.rb b/test/providers/user.rb
index 1244f6acd..1244f6acd 100644..100755
--- a/test/providers/user.rb
+++ b/test/providers/user.rb
diff --git a/test/puppet/tc_suidmanager.rb b/test/puppet/tc_suidmanager.rb
index c9b843147..c9b843147 100644..100755
--- a/test/puppet/tc_suidmanager.rb
+++ b/test/puppet/tc_suidmanager.rb
diff --git a/test/server/bucket.rb b/test/server/bucket.rb
index dcb50d27e..dcb50d27e 100644..100755
--- a/test/server/bucket.rb
+++ b/test/server/bucket.rb
diff --git a/test/server/ca.rb b/test/server/ca.rb
index 8be0df334..8be0df334 100644..100755
--- a/test/server/ca.rb
+++ b/test/server/ca.rb
diff --git a/test/server/logger.rb b/test/server/logger.rb
index 8ba0d0067..8ba0d0067 100644..100755
--- a/test/server/logger.rb
+++ b/test/server/logger.rb
diff --git a/test/server/master.rb b/test/server/master.rb
index b04fc92fd..b04fc92fd 100644..100755
--- a/test/server/master.rb
+++ b/test/server/master.rb
diff --git a/test/server/pelement.rb b/test/server/pelement.rb
index edfe89744..edfe89744 100644..100755
--- a/test/server/pelement.rb
+++ b/test/server/pelement.rb
diff --git a/test/server/server.rb b/test/server/server.rb
index 681fa1f42..681fa1f42 100644..100755
--- a/test/server/server.rb
+++ b/test/server/server.rb
diff --git a/test/tagging/tagging.rb b/test/tagging/tagging.rb
index 3044992d5..3044992d5 100644..100755
--- a/test/tagging/tagging.rb
+++ b/test/tagging/tagging.rb
diff --git a/test/types/basic.rb b/test/types/basic.rb
index e8dd38c77..e8dd38c77 100644..100755
--- a/test/types/basic.rb
+++ b/test/types/basic.rb
diff --git a/test/types/file.rb b/test/types/file.rb
index fba13bd12..1b305dfee 100644..100755
--- a/test/types/file.rb
+++ b/test/types/file.rb
@@ -1,3 +1,5 @@
+#!/usr/bin/env ruby -I../lib -I../../lib
+
 require 'puppet'
 require 'fileutils'
 require 'puppettest'
@@ -1466,6 +1468,125 @@ class TestFile < Test::Unit::TestCase
             Puppet::Type.type(:file).clear
         end
     end
+
+    # Testing #304
+    def test_links_to_directories
+        link = tempfile()
+        file = tempfile()
+        dir = tempfile()
+        Dir.mkdir(dir)
+
+        bucket = Puppet::Type.newfilebucket :name => "main"
+        File.symlink(dir, link)
+        File.open(file, "w") { |f| f.puts "" }
+        assert_equal(dir, File.readlink(link))
+        obj = Puppet::Type.newfile :path => link, :ensure => :link,
+            :target => file, :recurse => false, :backup => "main"
+
+        assert_apply(obj)
+
+        assert_equal(file, File.readlink(link))
+    end
+
+    # Testing #303
+    def test_nobackups_with_links
+        link = tempfile()
+        new = tempfile()
+
+        File.open(link, "w") { |f| f.puts "old" }
+        File.open(new, "w") { |f| f.puts "new" }
+        obj = Puppet::Type.newfile :path => link, :ensure => :link,
+            :target => new, :recurse => true, :backup => false
+
+        assert_nothing_raised do
+            obj.handlebackup
+        end
+
+        bfile = [link, "puppet-bak"].join(".")
+
+        assert(! FileTest.exists?(bfile), "Backed up when told not to")
+
+        assert_apply(obj)
+
+        assert(! FileTest.exists?(bfile), "Backed up when told not to")
+    end
+
+    def test_remove_existing
+    end
+
+    # Make sure we consistently handle backups for all cases.
+    def test_ensure_with_backups
+        # We've got three file types, so make sure we can replace any type
+        # with the other type and that backups are done correctly.
+        types = [:file, :directory, :link]
+
+        dir = tempfile()
+        path = File.join(dir, "test")
+        linkdest = tempfile()
+        creators = {
+            :file => proc { File.open(path, "w") { |f| f.puts "initial" } },
+            :directory => proc { Dir.mkdir(path) },
+            :link => proc { File.symlink(linkdest, path) }
+        }
+
+        bucket = Puppet::Type.newfilebucket :name => "main", :path => tempfile()
+
+        obj = Puppet::Type.newfile :path => path, :force => true,
+            :links => :manage
+
+        Puppet[:trace] = true
+        ["main", false].each do |backup|
+            obj[:backup] = backup
+            obj.finish
+            types.each do |should|
+                types.each do |is|
+                    # It makes no sense to replace a directory with a directory
+                    # next if should == :directory and is == :directory
+
+                    Dir.mkdir(dir)
+
+                    # Make the thing
+                    creators[is].call
+
+                    obj[:ensure] = should
+
+                    if should == :link
+                        obj[:target] = linkdest
+                    else
+                        if obj.state(:target)
+                            obj.delete(:target)
+                        end
+                    end
+
+                    # First try just removing the initial data
+                    assert_nothing_raised do
+                        obj.remove_existing(should)
+                    end
+
+                    unless is == should
+                        # Make sure the original is gone
+                        assert(! FileTest.exists?(obj[:path]),
+                            "remove_existing did not work: " +
+                            "did not remove %s with %s" % [is, should])
+                    end
+                    FileUtils.rmtree(obj[:path])
+
+                    # Now make it again
+                    creators[is].call
+
+                    state = obj.state(:ensure)
+
+                    state.retrieve
+                    unless state.insync?
+                        assert_nothing_raised do
+                            state.sync
+                        end
+                    end
+                    FileUtils.rmtree(dir)
+                end
+            end
+        end
+    end
 end
 
 # $Id$
diff --git a/test/types/fileignoresource.rb b/test/types/fileignoresource.rb
index 1340dbbc9..1340dbbc9 100644..100755
--- a/test/types/fileignoresource.rb
+++ b/test/types/fileignoresource.rb
diff --git a/test/types/package.rb b/test/types/package.rb
index a257a9324..a257a9324 100644..100755
--- a/test/types/package.rb
+++ b/test/types/package.rb
diff --git a/test/types/parameter.rb b/test/types/parameter.rb
index e94fe914b..e94fe914b 100644..100755
--- a/test/types/parameter.rb
+++ b/test/types/parameter.rb
diff --git a/test/types/query.rb b/test/types/query.rb
index fe5742490..fe5742490 100644..100755
--- a/test/types/query.rb
+++ b/test/types/query.rb
diff --git a/test/types/service.rb b/test/types/service.rb
index 79ec0c40c..79ec0c40c 100644..100755
--- a/test/types/service.rb
+++ b/test/types/service.rb
diff --git a/test/types/state.rb b/test/types/state.rb
index d4b5e91d5..d4b5e91d5 100644..100755
--- a/test/types/state.rb
+++ b/test/types/state.rb
diff --git a/test/types/type.rb b/test/types/type.rb
index e327a61a9..e327a61a9 100644..100755
--- a/test/types/type.rb
+++ b/test/types/type.rb
diff --git a/test/types/yumrepo.rb b/test/types/yumrepo.rb
index 4a377c3ed..4a377c3ed 100644..100755
--- a/test/types/yumrepo.rb
+++ b/test/types/yumrepo.rb