Fixing #542. Transactions now cause services to warn when they would have gotten restarted by a noop resource. Also fixing #549 -- transactions now only refuse to delete required resources if they are being purged. All other resources can be deleted just fine.

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

5 files changed, 74 insertions, 25 deletions

diff --git a/lib/puppet/propertychange.rb b/lib/puppet/propertychange.rb
index 0ae9b93d8..bbd18a22d 100644
--- a/lib/puppet/propertychange.rb
+++ b/lib/puppet/propertychange.rb
@@ -35,6 +35,23 @@ module Puppet
             self.changed
         end
 
+        # Create our event object.
+        def event(name)
+            # default to a simple event type
+            unless name.is_a?(Symbol)
+                @property.warning("Property '%s' returned invalid event '%s'; resetting to default" %
+                    [@property.class,event])
+
+                event = @property.parent.class.name.id2name + "_changed"
+            end
+            
+            Puppet::Event.new(
+                :event => name,
+                :transaction => @transaction,
+                :source => self.source
+            )
+        end
+
         def initialize(property)
             unless property.is_a?(Puppet::Type::Property)
                 raise Puppet::DevError, "Got a %s instead of a property" %
@@ -52,7 +69,13 @@ module Puppet
         # Perform the actual change.  This method can go either forward or
         # backward, and produces an event.
         def go
-            return nil if skip?
+            if skip?
+                if self.noop
+                    return [event(:noop)]
+                else
+                    return nil
+                end
+            end
 
             # The transaction catches any exceptions here.
             events = @property.sync
@@ -68,21 +91,9 @@ module Puppet
                 events = [events]
             end
             
-            return events.collect { |event|
-                # default to a simple event type
-                unless event.is_a?(Symbol)
-                    @property.warning("Property '%s' returned invalid event '%s'; resetting to default" %
-                        [@property.class,event])
-
-                    event = @property.parent.class.name.id2name + "_changed"
-                end
-                
+            return events.collect { |name|
                 @report = @property.log(@property.change_to_s)
-                Puppet::Event.new(
-                    :event => event,
-                    :transaction => @transaction,
-                    :source => self.source
-                )
+                event(name)
             }
         end
 
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index fed2348a9..fdc99ca21 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -40,9 +40,9 @@ class Transaction
         # If a resource is going to be deleted but it still has
         # dependencies, then don't delete it unless it's implicit or the
         # dependency is itself being deleted.
-        if ! resource.implicit? and resource.deleting?
+        if resource.purging? and resource.deleting?
             if deps = @relgraph.dependents(resource) and ! deps.empty? and deps.detect { |d| ! d.deleting? }
-                resource.warning "%s still depend%s on me -- not deleting" %
+                resource.warning "%s still depend%s on me -- not purging" %
                     [deps.collect { |r| r.ref }.join(","), deps.length > 1 ? "":"s"] 
                 return false
             end
@@ -80,7 +80,8 @@ class Transaction
 
         resourceevents = apply_changes(resource, changes)
 
-        unless changes.empty?
+        # If there were changes and the resource isn't in noop mode...
+        unless changes.empty? or changes.include?(:noop)
             # Record when we last synced
             resource.cache(:synced, Time.now)
 
@@ -95,7 +96,8 @@ class Transaction
                 # Create an edge with this resource as both the source and
                 # target.  The triggering method treats these specially for
                 # logging.
-                set_trigger(Puppet::Relationship.new(resource, resource, :callback => :refresh, :event => :ALL_EVENTS))
+                events = resourceevents.collect { |e| e.event }
+                set_trigger(Puppet::Relationship.new(resource, resource, :callback => :refresh, :event => events))
             end
         end
 
@@ -128,7 +130,7 @@ class Transaction
 
             # Mark that our change happened, so it can be reversed
             # if we ever get to that point
-            unless events.nil? or (events.is_a?(Array) and events.empty?)
+            unless events.nil? or (events.is_a?(Array) and (events.empty?) or events.include?(:noop))
                 change.changed = true
                 @resourcemetrics[:applied] += 1
             end
@@ -277,9 +279,10 @@ class Transaction
         # the parent resource in so it will override the source in the events,
         # since eval_generated children can't have direct relationships.
         @relgraph.matching_edges(events, resource).each do |edge|
-            unless edge
-                raise "wtf?"
-            end
+            edge = edge.dup
+            label = edge.label
+            label[:event] = events.collect { |e| e.event }
+            edge.label = label
             set_trigger(edge)
         end
 
@@ -677,6 +680,19 @@ class Transaction
         end
 
         callbacks.each do |callback, subs|
+            noop = true
+            subs.each do |edge|
+                if edge.event.nil? or ! edge.event.include?(:noop)
+                    noop = false
+                end
+            end
+
+            if noop
+                resource.notice "Would have triggered %s from %s dependencies" %
+                    [callback, subs.length]
+                return nil
+            end
+
             if subs.length == 1 and subs[0].source == resource
                 message = "Refreshing self"
             else
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index bf2060018..0144b517c 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -334,6 +334,21 @@ class Type < Puppet::Element
         self.class.self_refresh
     end
 
+    # Mark that we're purging.
+    def purging
+        @purging = true
+    end
+
+    # Is this resource being purged?  Used by transactions to forbid
+    # deletion when there are dependencies.
+    def purging?
+        if defined? @purging
+            @purging
+        else
+            false
+        end
+    end
+
     # Retrieve the title of an object.  If no title was set separately,
     # then use the object's name.
     def title
diff --git a/lib/puppet/type/property.rb b/lib/puppet/type/property.rb
index 2cc6beb03..4d772191d 100644
--- a/lib/puppet/type/property.rb
+++ b/lib/puppet/type/property.rb
@@ -305,8 +305,11 @@ class Property < Puppet::Parameter
         unless defined? @noop
             @noop = false
         end
-        tmp = @noop || self.parent.noop || Puppet[:noop] || false
-        #debug "noop is %s" % tmp
+        if self.parent.respond_to?(:noop)
+            tmp = @noop || self.parent.noop || Puppet[:noop] || false
+        else
+            tmp = @noop || Puppet[:noop] || false
+        end
         return tmp
     end
 
diff --git a/lib/puppet/type/resources.rb b/lib/puppet/type/resources.rb
index ea944d6b7..a3f6574d8 100644
--- a/lib/puppet/type/resources.rb
+++ b/lib/puppet/type/resources.rb
@@ -107,6 +107,10 @@ Puppet::Type.newtype(:resources) do
                 next unless param.metaparam?
                 resource[name] = param.value
             end
+
+            # Mark that we're purging, so transactions can handle relationships
+            # correctly
+            resource.purging
         end
     end