Source recursion is nearly working.

It works, but you have to run it multiple times,
and there are still a couple of strangenesses with the
parameter values, such as the mode not getting set on
the first run.

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

5 files changed, 183 insertions, 25 deletions

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index a59192af3..ce1df1c35 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -564,8 +564,9 @@ module Puppet
 
             # the right-side hash wins in the merge.
             options = to_hash.merge(:path => full_path, :implicit => true)
-            options.delete(:parent) if options.include?(:parent)
-            options.delete(:recurse) if options.include?(:recurse)
+            [:parent, :recurse, :target].each do |param|
+                options.delete(param) if options.include?(param)
+            end
 
             return self.class.create(options)
         end
@@ -632,6 +633,11 @@ module Puppet
         # Recurse the target of the link.
         def recurse_link(children)
             perform_recursion(self[:target]).each do |meta|
+                if meta.relative_path == "."
+                    self[:ensure] = :directory
+                    next
+                end
+
                 children[meta.relative_path] ||= newchild(meta.relative_path)
                 if meta.ftype == "directory"
                     children[meta.relative_path][:ensure] = :directory
@@ -645,7 +651,9 @@ module Puppet
 
         # Recurse the file itself, returning a Metadata instance for every found file.
         def recurse_local
-            perform_recursion(self[:path]).inject({}) do |hash, meta|
+            result = perform_recursion(self[:path])
+            return {} unless result
+            result.inject({}) do |hash, meta|
                 next hash if meta.relative_path == "."
 
                 hash[meta.relative_path] = newchild(meta.relative_path)
@@ -675,8 +683,14 @@ module Puppet
             end
 
             total.each do |meta|
+                if meta.relative_path == "."
+                    property(:source).metadata = meta
+                    next
+                end
                 children[meta.relative_path] ||= newchild(meta.relative_path)
                 children[meta.relative_path][:source] = meta.source
+                children[meta.relative_path][:checksum] = :md5 if meta.ftype == "file"
+
                 children[meta.relative_path].property(:source).metadata = meta
             end
 
@@ -759,21 +773,22 @@ module Puppet
         # a wrapper method to make sure the file exists before doing anything
         def retrieve
             unless stat = self.stat(true)
-                # If the file doesn't exist but we have a source, then call
-                # retrieve on that property
 
                 propertyvalues = properties().inject({}) { |hash, property|
                                      hash[property] = :absent
                                      hash
                                   }
 
+                # If the file doesn't exist but we have a source, then call
+                # retrieve on the source property so it will set the 'should'
+                # values all around.
                 if @parameters.include?(:source)
-                    propertyvalues[:source] = @parameters[:source].retrieve
+                    @parameters[:source].copy_source_values
                 end
                 return propertyvalues
             end
 
-            return currentpropvalues()
+            currentpropvalues()
         end
 
         # This recurses against the remote source and makes sure the local
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 03cc311b4..e43706051 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -122,13 +122,14 @@ module Puppet
 
             # Take each of the stats and set them as states on the local file
             # if a value has not already been provided.
-            [:owner, :mode, :group].each do |param|
-                @resource[param] ||= metadata.send(param)
+            [:owner, :mode, :group, :checksum].each do |param|
+                next if param == :owner and Puppet::Util::SUIDManager.uid != 0
+                unless value = @resource[param] and value != :absent
+                    @resource[param] = metadata.send(param)
+                end
             end
 
-            unless @resource.deleting?
-                @resource[:ensure] = metadata.ftype
-            end
+            @resource[:ensure] = metadata.ftype
 
             if metadata.ftype == "link"
                 @resource[:target] = metadata.destination
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index 39d0b62dc..b59fa3294 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -28,6 +28,19 @@ describe Puppet::Type.type(:file), "when recursing" do
         end
     end
 
+    it "should be able to recurse over a nonexistent file" do
+        @path = Tempfile.new("file_integration_tests")
+        @path.close!()
+        @path = @path.path
+
+        @file = Puppet::Type::File.create(:name => @path, :mode => 0644, :recurse => true)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        lambda { @file.eval_generate }.should_not raise_error
+    end
+
     it "should be able to recursively set properties on existing files" do
         @path = Tempfile.new("file_integration_tests")
         @path.close!()
@@ -50,4 +63,66 @@ describe Puppet::Type.type(:file), "when recursing" do
             (File.stat(path).mode & 007777).should == 0644
         end
     end
+
+    it "should be able to recursively make links to other files" do
+        source = Tempfile.new("file_link_integration_source")
+        source.close!()
+        source = source.path
+
+        build_path(source)
+
+        dest = Tempfile.new("file_link_integration_dest")
+        dest.close!()
+        dest = dest.path
+
+        @file = Puppet::Type::File.create(:name => dest, :target => source, :recurse => true, :ensure => :link)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        @catalog.apply
+
+        @dirs.each do |path|
+            link_path = path.sub(source, dest)
+
+            File.lstat(link_path).should be_directory
+        end
+
+        @files.each do |path|
+            link_path = path.sub(source, dest)
+
+            File.lstat(link_path).ftype.should == "link"
+        end
+    end
+
+    it "should be able to recursively copy files" do
+        source = Tempfile.new("file_source_integration_source")
+        source.close!()
+        source = source.path
+
+        build_path(source)
+
+        dest = Tempfile.new("file_source_integration_dest")
+        dest.close!()
+        dest = dest.path
+
+        @file = Puppet::Type::File.create(:name => dest, :source => source, :recurse => true)
+
+        @catalog = Puppet::Node::Catalog.new
+        @catalog.add_resource @file
+
+        @catalog.apply
+
+        @dirs.each do |path|
+            newpath = path.sub(source, dest)
+
+            File.lstat(newpath).should be_directory
+        end
+
+        @files.each do |path|
+            newpath = path.sub(source, dest)
+
+            File.lstat(newpath).ftype.should == "file"
+        end
+    end
 end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 9578a2d17..327c49d9c 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -137,6 +137,11 @@ describe Puppet::Type.type(:file) do
             @file.recurse_local
         end
 
+        it "should return an empty hash if the recursion returns nothing" do
+            @file.expects(:perform_recursion).returns nil
+            @file.recurse_local.should == {}
+        end
+
         it "should create a new child resource with each generated metadata instance's relative path" do
             @file.expects(:perform_recursion).returns [@metadata]
             @file.expects(:newchild).with(@metadata.relative_path).returns "fiebar"
@@ -177,6 +182,15 @@ describe Puppet::Type.type(:file) do
             @file.recurse_link({})
         end
 
+        it "should ignore the recursively-found '.' file and configure the top-level file to create a directory" do
+            @first.stubs(:relative_path).returns "."
+            @file[:target] = "mylinks"
+            @file.expects(:perform_recursion).with("mylinks").returns [@first]
+            @file.stubs(:newchild).never
+            @file.expects(:[]=).with(:ensure, :directory)
+            @file.recurse_link({})
+        end
+
         it "should create a new child resource for each generated metadata instance's relative path that doesn't already exist in the children hash" do
             @file.expects(:perform_recursion).returns [@first, @second]
             @file.expects(:newchild).with(@first.relative_path).returns @resource
@@ -258,10 +272,21 @@ describe Puppet::Type.type(:file) do
 
         it "should set the source of each resource to the source of the metadata" do
             @file.stubs(:perform_recursion).returns [@first]
+            @resource.stubs(:[]=)
             @resource.expects(:[]=).with(:source, File.join("puppet://foo/bar", @first.relative_path))
             @file.recurse_remote("first" => @resource)
         end
 
+        # LAK:FIXME This is a bug, but I can't think of a fix for it.  Fortunately it's already
+        # filed, and when it's fixed, we'll just fix the whole flow.
+        it "should set the checksum type to :md5 if the remote file is a file" do
+            @first.stubs(:ftype).returns "file"
+            @file.stubs(:perform_recursion).returns [@first]
+            @resource.stubs(:[]=)
+            @resource.expects(:[]=).with(:checksum, :md5)
+            @file.recurse_remote("first" => @resource)
+        end
+
         it "should store the metadata in the source property for each resource so the source does not have to requery the metadata" do
             @file.stubs(:perform_recursion).returns [@first]
             @resource.expects(:property).with(:source).returns @property
@@ -271,6 +296,24 @@ describe Puppet::Type.type(:file) do
             @file.recurse_remote("first" => @resource)
         end
 
+        it "should not create a new resource for the '.' file" do
+            @first.stubs(:relative_path).returns "."
+            @file.stubs(:perform_recursion).returns [@first]
+
+            @file.expects(:newchild).never
+
+            @file.recurse_remote({})
+        end
+
+        it "should store the metadata in the main file's source property if the relative path is '.'" do
+            @first.stubs(:relative_path).returns "."
+            @file.stubs(:perform_recursion).returns [@first]
+
+            @file.property(:source).expects(:metadata=).with @first
+            
+            @file.recurse_remote("first" => @resource)
+        end
+
         describe "and purging is enabled" do
             before do
                 @file[:purge] = true
@@ -464,6 +507,12 @@ describe Puppet::Type.type(:file) do
                 Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:recurse) }
                 @file.newchild("my/path")
             end
+
+            it "should not copy the parent resource's target value" do
+                @file.expects(:to_hash).returns :target => "foo"
+                Puppet::Type.type(:file).expects(:create).with { |options| ! options.include?(:target) }
+                @file.newchild("my/path")
+            end
         end
     end
 end
diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb
index 7c9850822..2883e9c6b 100755
--- a/spec/unit/type/file/source.rb
+++ b/spec/unit/type/file/source.rb
@@ -99,7 +99,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
     describe "when copying the source values" do
         before do
-            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123
+            @metadata = stub 'metadata', :owner => 100, :group => 200, :mode => 123, :checksum => "{md5}asdfasdf"
 
             @source = source.new(:resource => @resource)
             @source.metadata = @metadata
@@ -113,8 +113,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             lambda { @source.copy_source_values }.should raise_error(ArgumentError)
         end
 
-        it "should set :ensure to the file type if the resource is not being deleted" do
-            @resource.expects(:deleting?).returns false
+        it "should set :ensure to the file type" do
             @resource.stubs(:[])
             @resource.stubs(:[]=)
             @metadata.stubs(:ftype).returns "foobar"
@@ -123,16 +122,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             @source.copy_source_values
         end
 
-        it "should not set :ensure to the file type if the resource is being deleted" do
-            @resource.expects(:deleting?).returns true
-            @resource.stubs(:[])
-            @resource.stubs(:[]).returns "foo"
-            @metadata.expects(:ftype).returns "foobar"
-
-            @resource.expects(:[]=).with(:ensure, "foobar").never
-            @source.copy_source_values
-        end
-
         describe "and the source is a file" do
             before do
                 @metadata.stubs(:ftype).returns "file"
@@ -141,9 +130,25 @@ describe Puppet::Type.type(:file).attrclass(:source) do
             it "should copy the metadata's owner, group, and mode to the resource if they are not set on the resource" do
                 @resource.stubs(:[]).returns nil
 
+                Puppet::Util::SUIDManager.expects(:uid).returns 0
+
+                @resource.expects(:[]=).with(:owner, 100)
+                @resource.expects(:[]=).with(:group, 200)
+                @resource.expects(:[]=).with(:mode, 123)
+                @resource.expects(:[]=).with(:checksum, "{md5}asdfasdf")
+
+                @source.copy_source_values
+            end
+
+            it "should copy the metadata's owner, group, and mode to the resource if they are set to :absent on the resource" do
+                @resource.stubs(:[]).returns :absent
+
+                Puppet::Util::SUIDManager.expects(:uid).returns 0
+
                 @resource.expects(:[]=).with(:owner, 100)
                 @resource.expects(:[]=).with(:group, 200)
                 @resource.expects(:[]=).with(:mode, 123)
+                @resource.expects(:[]=).with(:checksum, "{md5}asdfasdf")
 
                 @source.copy_source_values
             end
@@ -156,6 +161,19 @@ describe Puppet::Type.type(:file).attrclass(:source) do
 
                 @source.copy_source_values
             end
+
+            describe "and puppet is not running as root" do
+                it "should not try to set the owner" do
+                    @resource.stubs(:[]).returns nil
+                    @resource.stubs(:[]=)
+
+                    @resource.expects(:[]=).with(:owner, 100).never
+
+                    Puppet::Util::SUIDManager.expects(:uid).returns 100
+
+                    @source.copy_source_values
+                end
+            end
         end
 
         describe "and the source is a link" do