Fixed #1037 -- remote unreadable files no longer have the

permission denied exceptions caught, thus forbidding them
from being replaced with 'nil'.

4 files changed, 64 insertions, 49 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 01dd986e2..3d08f305a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,7 @@
+    Fixed #1037 -- remote unreadable files no longer have the
+    permission denied exceptions caught, thus forbidding them
+    from being replaced with 'nil'.
+
     Fixed #1043 -- autoloading now searches the plugins directory
     in each module, in addition to the lib directory.  The 'lib'
     directory is also deprecated, but supported for now to give
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 65ee7e72e..0ec54d907 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -642,7 +642,6 @@ module Puppet
                     # :file.
                     return nil unless child = catalog.create_implicit_resource(self.class.name, args)
                 rescue => detail
-                    puts detail.backtrace
                     self.notice "Cannot manage: %s" % [detail]
                     return nil
                 end
@@ -769,11 +768,8 @@ module Puppet
             begin
                 File.unlink(newfile)
             rescue => detail
-                if Puppet[:trace]
-                    puts detail.backtrace
-                end
-                self.err "Could not remove old backup: %s" %
-                    detail
+                puts detail.backtrace if Puppet[:trace]
+                self.err "Could not remove old backup: %s" % detail
                 return false
             end
         end
@@ -979,7 +975,7 @@ module Puppet
         end
 
         def uri2obj(source)
-            sourceobj = FileSource.new
+            sourceobj = Puppet::Type::File::FileSource.new
             path = nil
             unless source
                 devfail "Got a nil source"
@@ -1152,7 +1148,7 @@ module Puppet
 
     # the filesource class can't include the path, because the path
     # changes for every file instance
-    class FileSource
+    class ::Puppet::Type::File::FileSource
         attr_accessor :mount, :root, :server, :local
     end
 
diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb
index 3dfb5cccd..a3e533c31 100755
--- a/lib/puppet/type/file/source.rb
+++ b/lib/puppet/type/file/source.rb
@@ -101,8 +101,7 @@ module Puppet
             begin
                 desc = server.describe(path, @resource[:links])
             rescue Puppet::Network::XMLRPCClientError => detail
-                self.err "Could not describe %s: %s" %
-                    [path, detail]
+                self.err "Could not describe %s: %s" % [path, detail]
                 return nil
             end
 
@@ -163,7 +162,7 @@ module Puppet
 
             # Diff the contents if they ask it.  This is quite annoying -- we need to do this in
             # 'insync?' because they might be in noop mode, but we don't want to do the file
-            # retrieval twice, so we cache the value annoyingly.
+            # retrieval twice, so we cache the value.
             if ! result and Puppet[:show_diff] and File.exists?(@resource[:path]) and ! @stats[:_diffed]
                 @stats[:_remote_content] = get_remote_content
                 string_file_diff(@resource[:path], @stats[:_remote_content])
@@ -203,13 +202,10 @@ module Puppet
             
             case @stats[:type]
             when "directory", "file":
-                unless @resource.deleting?
-                    @resource[:ensure] = @stats[:type]
-                end
+                @resource[:ensure] = @stats[:type] unless @resource.deleting?
             else
                 self.info @stats.inspect
-                self.err "Cannot use files of type %s as sources" %
-                    @stats[:type]
+                self.err "Cannot use files of type %s as sources" % @stats[:type]
                 return :nocopy
             end
 
@@ -221,11 +217,9 @@ module Puppet
 
                 # was the stat already specified, or should the value
                 # be inherited from the source?
-                unless @resource.argument?(stat)
-                    @resource[stat] = value
-                end
+                @resource[stat] = value unless @resource.argument?(stat)
             }
-            
+
             return @stats[:checksum]
         end
         
@@ -261,34 +255,22 @@ module Puppet
         end
 
         private
+
         def get_remote_content
-            unless @stats[:type] == "file"
-                #if @stats[:type] == "directory"
-                        #[@resource.name, @should.inspect]
-                #end
-                raise Puppet::DevError, "Got told to copy non-file %s" %
-                    @resource[:path]
-            end
+            raise Puppet::DevError, "Got told to copy non-file %s" % @resource[:path] unless @stats[:type] == "file"
 
             sourceobj, path = @resource.uri2obj(@source)
 
             begin
                 contents = sourceobj.server.retrieve(path, @resource[:links])
-            rescue Puppet::Network::XMLRPCClientError => detail
-                self.err "Could not retrieve %s: %s" %
-                    [path, detail]
-                return nil
+            rescue => detail
+                self.fail "Could not retrieve %s: %s" % [path, detail]
             end
 
-            # FIXME It's stupid that this isn't taken care of in the
-            # protocol.
-            unless sourceobj.server.local
-                contents = CGI.unescape(contents)
-            end
+            contents = CGI.unescape(contents) unless sourceobj.server.local
 
             if contents == ""
-                self.notice "Could not retrieve contents for %s" %
-                    @source
+                self.notice "Could not retrieve contents for %s" % @source
             end
 
             return contents
diff --git a/spec/unit/ral/types/file.rb b/spec/unit/ral/types/file.rb
index 7202e23e5..9ff6dc977 100755
--- a/spec/unit/ral/types/file.rb
+++ b/spec/unit/ral/types/file.rb
@@ -4,26 +4,59 @@ require File.dirname(__FILE__) + '/../../../spec_helper'
 
 require 'puppet/type/file'
 
-describe Puppet::Type::File, " when used with replace=>false and content" do
+describe Puppet::Type::File do
     before do
         @path = Tempfile.new("puppetspec")
         @path.close!()
         @path = @path.path
-        @file = Puppet::Type::File.create( { :name => @path, :content => "foo", :replace => :false } )
+        @file = Puppet::Type::File.create(:name => @path)
     end
 
-    it "should be insync if the file exists and the content is different" do
-        File.open(@path, "w") do |f| f.puts "bar" end
-        @file.property(:content).insync?("bar").should be_true
-    end
+    describe "when used with content and replace=>false" do
+        before do
+            @file[:content] = "foo"
+            @file[:replace] = false
+        end
+
+        it "should be insync if the file exists and the content is different" do
+            File.open(@path, "w") do |f| f.puts "bar" end
+            @file.property(:content).insync?("bar").should be_true
+        end
 
-    it "should be insync if the file exists and the content is right" do
-        File.open(@path, "w") do |f| f.puts "foo" end
-        @file.property(:content).insync?("foo").should be_true
+        it "should be insync if the file exists and the content is right" do
+            File.open(@path, "w") do |f| f.puts "foo" end
+            @file.property(:content).insync?("foo").should be_true
+        end
+
+        it "should not be insync if the file does not exist" do
+            @file.property(:content).insync?(:nil).should be_false
+        end
     end
 
-    it "should not be insync if the file doesnot exist" do
-        @file.property(:content).insync?(:nil).should be_false
+    describe "when retrieving remote files" do
+        before do
+            @filesource = Puppet::Type::File::FileSource.new
+            @filesource.server = mock 'fileserver'
+
+            @file.stubs(:uri2obj).returns(@filesource)
+
+            @file[:source] = "puppet:///test"
+        end
+
+        it "should fail without writing if it cannot retrieve remote contents" do
+            # create the file, because we only get the problem when it starts
+            # out absent.
+            File.open(@file[:path], "w") { |f| f.puts "a" }
+            Puppet::Util::Log.level = :debug
+            Puppet::Util::Log.newdestination :console
+            @file.expects(:write).never
+
+
+            @filesource.server.stubs(:describe).returns("493\tfile\t100\t0\t{md5}3f5fef3bddbc4398c46a7bd7ba7b3af7")
+            @filesource.server.stubs(:retrieve).raises(RuntimeError)
+            @file.property(:source).retrieve
+            lambda { @file.property(:source).sync }.should raise_error(Puppet::Error)
+        end
     end
 
     after do