Fixing #2253 - pluginsync failures propagate correctly

This keeps the destination directory from getting
purged if the remote source is invalid.

This mostly just removes an optimization that worked
fine when we queried the server for every directory,
but doesn't work now that we do one big query.

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

2 files changed, 33 insertions, 8 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 1f4cb3edc..dd8689447 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -192,17 +192,17 @@ class Transaction
     end
     
     # Evaluate a single resource.
-    def eval_resource(resource, checkskip = true)
+    def eval_resource(resource)
         events = []
         
         if resource.is_a?(Puppet::Type::Component)
             raise Puppet::DevError, "Got a component to evaluate"
         end
         
-        if checkskip and skip?(resource)
+        if skip?(resource)
             @resourcemetrics[:skipped] += 1
         else
-            events += eval_children_and_apply_resource(resource, checkskip)
+            events += eval_children_and_apply_resource(resource)
         end
 
         # Check to see if there are any events for this resource
@@ -226,7 +226,7 @@ class Transaction
         events
     end
 
-    def eval_children_and_apply_resource(resource, checkskip)
+    def eval_children_and_apply_resource(resource)
         events = []
 
         @resourcemetrics[:scheduled] += 1
@@ -237,7 +237,7 @@ class Transaction
         # actions sometimes change how the top resource is applied.
         children = eval_generate(resource)
         
-        if children and resource.depthfirst?
+        if ! children.empty? and resource.depthfirst?
             children.each do |child|
                 # The child will never be skipped when the parent isn't
                 events += eval_resource(child, false)
@@ -249,9 +249,9 @@ class Transaction
             events += apply(resource)
         end
 
-        if children and ! resource.depthfirst?
+        if ! children.empty? and ! resource.depthfirst?
             children.each do |child|
-                events += eval_resource(child, false)
+                events += eval_resource(child)
             end
         end
 
@@ -260,7 +260,7 @@ class Transaction
         # the changes list this resource as a proxy.  This is really only
         # necessary for rollback, since we know the generating resource
         # during forward changes.
-        if children and checkskip
+        unless children.empty?
             @changes[changecount..-1].each { |change| change.proxy = resource }
         end
 
diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb
new file mode 100755
index 000000000..17aba0df8
--- /dev/null
+++ b/spec/integration/transaction.rb
@@ -0,0 +1,25 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+require 'puppet/transaction'
+
+describe Puppet::Transaction do
+    it "should not apply generated resources if the parent resource fails" do
+        catalog = Puppet::Resource::Catalog.new
+        resource = Puppet::Type.type(:file).new :path => "/foo/bar"
+        catalog.add_resource resource
+
+        child_resource = Puppet::Type.type(:file).new :path => "/foo/bar/baz"
+
+        resource.expects(:eval_generate).returns([child_resource])
+
+        transaction = Puppet::Transaction.new(catalog)
+
+        resource.expects(:evaluate).raises "this is a failure"
+
+        child_resource.expects(:evaluate).never
+
+        transaction.evaluate
+    end
+end