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

10 files changed, 170 insertions, 55 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8a7ac586b..bb64b1de7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,4 +1,12 @@
 0.22.2 (grover)
+    Resources can now be freely deleted, thus fixing many problems introduced
+    when deletion of required resources was forbidden when purging was introduced.
+    Only resources being purged will not be deleted.
+
+    Facts and plugins now download even in noop mode (#540).
+
+    Resources in noop mode now log when they would have responded to an event (#542).
+
     Refactored cron support entirely.  Cron now uses providers, and there
     is a single 'crontab' provider that handles user crontabs.  While this
     refactor does not include providers for /etc/crontab or cron.d, it should
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
     
diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb
index 1fa18c3b6..6f543292a 100755
--- a/test/lib/puppettest.rb
+++ b/test/lib/puppettest.rb
@@ -163,10 +163,10 @@ module PuppetTest
             if defined? $console
                 Puppet.info @method_name
                 Puppet::Util::Log.newdestination(:console)
+                Puppet[:trace] = true
             end
             Puppet::Util::Log.level = :debug
             #$VERBOSE = 1
-            Puppet[:trace] = true
         else    
             Puppet::Util::Log.close
             Puppet::Util::Log.newdestination(@logs)
diff --git a/test/other/propertychange.rb b/test/other/propertychange.rb
index e3df577d2..d60a2ba2c 100755
--- a/test/other/propertychange.rb
+++ b/test/other/propertychange.rb
@@ -11,6 +11,7 @@ class TestPropertyChange < Test::Unit::TestCase
 	include PuppetTest
 	class FakeProperty < Puppet::Type::Property
 	    attr_accessor :is, :should, :parent
+	    attr_reader :noop
 	    def change_to_s
 	        "fake change"
         end
@@ -25,11 +26,18 @@ class TestPropertyChange < Test::Unit::TestCase
             )
         end
         def noop
-            false
+            if defined? @noop
+                @noop
+            else
+                false
+            end
         end
         def path
             "fakechange"
         end
+        def should_to_s
+            @should.to_s
+        end
         def sync
             if insync?
                 return nil
@@ -107,6 +115,28 @@ class TestPropertyChange < Test::Unit::TestCase
 	    #assert(coll.detect { |l| l.message == "fake change" }, "Did not log change")
 	    assert_equal(change.property.is, change.property.should, "did not call sync method")
     end
+
+    # Related to #542.  Make sure changes in noop mode produce the :noop event.
+    def test_noop_event
+        change = mkchange
+
+        assert(! change.skip?, "Change is already being skipped")
+
+        Puppet[:noop] = true
+
+        change.property.noop = true
+        p change.property.noop
+        assert(change.noop, "did not set noop")
+        assert(change.skip?, "setting noop did not mark change for skipping")
+
+        event = nil
+        assert_nothing_raised("Could not generate noop event") do
+            event = change.forward
+        end
+
+        assert_equal(1, event.length, "got wrong number of events")
+        assert_equal(:noop, event[0].event, "did not generate noop mode when in noop")
+    end
 end
 
 # $Id$
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index 1213c75b9..d9451291e 100755
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -13,6 +13,9 @@ class TestTransactions < Test::Unit::TestCase
     include PuppetTest::Support::Resources
     class Fakeprop <Puppet::Type::Property
         attr_accessor :path, :is, :should, :name
+        def should_to_s
+            @should.to_s
+        end
         def insync?
             true
         end
@@ -756,34 +759,6 @@ class TestTransactions < Test::Unit::TestCase
         assert(trans.tagged?(res), "tags should have matched")
     end
     
-    # We don't want to purge resources that have relationships with other
-    # resources, so we want our transactions to check for that.
-    def test_required_resources_not_deleted
-        @file = Puppet::Type.type(:file)
-        path1 = tempfile()
-        path2 = tempfile()
-        
-        # Create our first file
-        File.open(path1, "w") { |f| f.puts "yay" }
-        
-        # Create a couple of related resources
-        file1 = @file.create :title => "dependee", :path => path1, :ensure => :absent
-        file2 = @file.create :title => "depender", :path => path2, :content => "some stuff", :require => file1
-                
-        # Now make sure we don't actually delete the first file
-        assert_apply(file1, file2)
-        
-        assert(FileTest.exists?(path1), "required file was deleted")
-
-        # However, we *do* want to allow deletion of resources of their dependency
-        # is also being deleted
-        file2[:ensure] = :absent
-
-        assert_apply(file1, file2)
-        
-        assert(! FileTest.exists?(path1), "dependency blocked deletion even when it was itself being deleted")
-    end
-    
     # Make sure changes generated by eval_generated resources have proxies
     # set to the top-level resource.
     def test_proxy_resources
@@ -1109,6 +1084,55 @@ class TestTransactions < Test::Unit::TestCase
         assert_equal(:refresh, label[:callback],
             "did not get correct callback from notify")
     end
+
+    # #542 - make sure resources in noop mode still notify their resources,
+    # so that users know if a service will get restarted.
+    def test_noop_with_notify
+        path = tempfile
+        epath = tempfile
+        file = Puppet::Type.newfile(:path => path, :ensure => :file)
+        exec = Puppet::Type.type(:exec).create(:command => "touch %s" % epath,
+            :path => ENV["PATH"], :subscribe => file, :refreshonly => true)
+
+        Puppet[:noop] = true
+
+        assert(file.noop, "file not in noop")
+        assert(exec.noop, "exec not in noop")
+
+        assert_apply(file, exec)
+
+        assert(! FileTest.exists?(path), "Created file in noop")
+        assert(! FileTest.exists?(epath), "Executed exec in noop")
+
+        logs = @logs.dup
+        assert_logged(:notice, /should be/, "did not log file change")
+        @logs = logs
+        assert_logged(:notice, /Would have triggered/, "did not log exec trigger")
+    end
+
+    def test_only_stop_purging_with_relations
+        files = []
+        paths = []
+        3.times do |i|
+            path = tempfile
+            paths << path
+            file = Puppet::Type.newfile(:path => path, :ensure => :absent,
+                :backup => false, :title => "file%s" % i)
+            File.open(path, "w") { |f| f.puts "" }
+            files << file
+        end
+
+        files[0][:ensure] = :file
+        files[0][:require] = files[1..2]
+
+        # Mark the second as purging
+        files[1].purging
+
+        assert_apply(*files)
+
+        assert(FileTest.exists?(paths[1]), "Deleted required purging file")
+        assert(! FileTest.exists?(paths[2]), "Did not delete non-purged file")
+    end
 end
 
 # $Id$
diff --git a/test/ral/types/resources.rb b/test/ral/types/resources.rb
index efd84a431..bc6c9f6a9 100755
--- a/test/ral/types/resources.rb
+++ b/test/ral/types/resources.rb
@@ -134,6 +134,10 @@ class TestResources < Test::Unit::TestCase
             genned = purger.generate
         end
         assert(genned, "Did not get any generated resources")
+
+        genned.each do |res|
+            assert(res.purging, "did not mark resource for purging")
+        end
         assert(! genned.empty?, "generated resource list was empty")
         
         # Now make sure the generate method only finds the unmanaged resources