From ad90900e68a2c406f0e95dba3f780ad135415b14 Mon Sep 17 00:00:00 2001
From: Luke Kanies <luke@madstop.com>
Date: Thu, 29 Oct 2009 09:21:34 -0700
Subject: Random code cleanup

Signed-off-by: Luke Kanies <luke@madstop.com>
---
 lib/puppet/property.rb           |   6 +-
 lib/puppet/transaction.rb        |  50 +++++---------
 lib/puppet/transaction/change.rb |  59 ++++++++---------
 spec/unit/transaction.rb         |  94 +++++++++++----------------
 spec/unit/transaction/change.rb  | 136 +++++++++++++++++++++++++--------------
 5 files changed, 175 insertions(+), 170 deletions(-)

diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 753c0d7c6..e246eb702 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -152,8 +152,8 @@ class Puppet::Property < Puppet::Parameter
     end
 
     # Figure out which event to return.
-    def event(name, event = nil)
-        if value_event = self.class.value_option(name, :event)
+    def default_event_name(value, event = nil)
+        if value_event = self.class.value_option(value, :event)
             return value_event
         end
 
@@ -315,7 +315,7 @@ class Puppet::Property < Puppet::Parameter
             devfail "Cannot use obsolete :call value '%s' for property '%s'" % [call, self.class.name]
         end
 
-        return event(name, event)
+        return default_event_name(name, event)
     end
 
     # If there's a shadowing metaparam, instantiate it now.
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 6671fb1f0..a5feecbc2 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -409,7 +409,7 @@ class Puppet::Transaction
         end
 
         if restarted
-            queue_events(Puppet::Transaction::Event.new(:restarted, resource))
+            queue_event(resource, Puppet::Transaction::Event.new(:restarted, resource))
 
             @resourcemetrics[:restarted] += 1
         end
@@ -417,12 +417,8 @@ class Puppet::Transaction
 
     # Queue events for other resources to respond to.  All of these events have
     # to be from the same resource.
-    def queue_events(*events)
-        events.flatten!
-
-        @events += events
-
-        resource = events[0].resource
+    def queue_event(resource, event)
+        @events << event
 
         # Collect the targets of any subscriptions to those events.  We pass
         # the parent resource in so it will override the source in the events,
@@ -431,20 +427,20 @@ class Puppet::Transaction
             next unless method = edge.callback
             next unless edge.target.respond_to?(method)
 
-            queue_events_for_resource(resource, edge.target, method, events)
+            queue_event_for_resource(resource, edge.target, method, event)
         end
 
         if resource.self_refresh? and ! resource.deleting?
-            queue_events_for_resource(resource, resource, :refresh, events)
+            queue_event_for_resource(resource, resource, :refresh, event)
         end
     end
 
-    def queue_events_for_resource(source, target, callback, events)
+    def queue_event_for_resource(source, target, callback, event)
         source.info "Scheduling #{callback} of #{target}"
 
         @event_queues[target] ||= {}
         @event_queues[target][callback] ||= []
-        @event_queues[target][callback] += events
+        @event_queues[target][callback] << event
     end
 
     def queued_events(resource)
@@ -461,13 +457,8 @@ class Puppet::Transaction
     # Roll all completed changes back.
     def rollback
         @changes.reverse.collect do |change|
-            # skip changes that were never actually run
-            unless change.changed
-                Puppet.debug "%s was not changed" % change.to_s
-                next
-            end
             begin
-                events = change.backward
+                event = change.backward
             rescue => detail
                 Puppet.err("%s rollback failed: %s" % [change,detail])
                 if Puppet[:trace]
@@ -483,7 +474,7 @@ class Puppet::Transaction
             end
 
             # And queue the events
-            queue_events(events)
+            queue_event(change.resource, event)
 
             # Now check to see if there are any events for this child.
             process_events(change.property.resource)
@@ -542,21 +533,14 @@ class Puppet::Transaction
     def apply_change(resource, change)
         @changes << change
 
-        # use an array, so that changes can return more than one
-        # event if they want
-        events = [change.forward].flatten.reject { |e| e.nil? }
+        event = change.forward
 
-        # Mark that our change happened, so it can be reversed
-        # if we ever get to that point
-        change.changed = true
-        @resourcemetrics[:applied] += 1
-        queue_events(events)
-    rescue => detail
-        puts detail.backtrace if Puppet[:trace]
-        is = change.property.is_to_s(change.is)
-        should = change.property.should_to_s(change.should)
-        change.property.err "change from #{is} to #{should} failed: #{detail}"
-        @failures[resource] += 1
+        if event.status == "success"
+            @resourcemetrics[:applied] += 1
+        else
+            @failures[resource] += 1
+        end
+        queue_event(resource, event)
     end
 
     def process_callback(resource, callback, events)
@@ -577,7 +561,7 @@ class Puppet::Transaction
         resource.notice "Would have triggered '#{callback}' from #{events.length} events"
 
         # And then add an event for it.
-        queue_events(Puppet::Transaction::Event.new(:noop, resource))
+        queue_event(resource, Puppet::Transaction::Event.new(:noop, resource))
         true # so the 'and if' works
     end
 end
diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index b710ee08d..5e1aff106 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -4,7 +4,7 @@ require 'puppet/transaction/event'
 # Handle all of the work around performing an actual change,
 # including calling 'sync' on the properties and producing events.
 class Puppet::Transaction::Change
-    attr_accessor :is, :should, :path, :property, :changed, :proxy
+    attr_accessor :is, :should, :property, :proxy
 
     # Switch the goals of the property, thus running the change in reverse.
     def backward
@@ -15,26 +15,22 @@ class Puppet::Transaction::Change
         return self.go
     end
 
-    def changed?
-        self.changed
-    end
-
     # Create our event object.
-    def event(name)
+    def event(event_name)
+        event_name ||= property.default_event_name(should)
+
         # default to a simple event type
-        unless name.is_a?(Symbol)
-            @property.warning("Property '%s' returned invalid event '%s'; resetting to default" %
-                [@property.class, name])
+        unless event_name.is_a?(Symbol)
+            @property.warning "Property '#{property.class}' returned invalid event '#{event_name}'; resetting to default"
 
-            name = @property.event(should)
+            event_name = property.default_event_name(should)
         end
 
-        Puppet::Transaction::Event.new(name, self.resource)
+        Puppet::Transaction::Event.new(event_name, resource.ref, property.name, is, should)
     end
 
     def initialize(property, currentvalue)
         @property = property
-        @path = [property.path,"change"].flatten
         @is = currentvalue
 
         @should = property.should
@@ -47,25 +43,30 @@ class Puppet::Transaction::Change
     def go
         if self.noop?
             @property.log "is %s, should be %s (noop)" % [property.is_to_s(@is), property.should_to_s(@should)]
-            return [event(:noop)]
+            return event(:noop)
         end
 
         # The transaction catches any exceptions here.
-        events = @property.sync
-        if events.nil?
-            events = [(@property.name.to_s + "_changed").to_sym]
-        elsif events.is_a?(Array)
-            if events.empty?
-                events = [(@property.name.to_s + "_changed").to_sym]
-            end
-        else
-            events = [events]
-        end
-
-        return events.collect { |name|
-            @report = @property.log(@property.change_to_s(@is, @should))
-            event(name)
-        }
+        event_name = @property.sync
+
+        # Use the first event only, if multiple are provided.
+        # This might result in the event_name being nil,
+        # which is fine.
+        event_name = event_name.shift if event_name.is_a?(Array)
+
+        event = event(event_name)
+        event.log = @property.notice @property.change_to_s(@is, @should)
+        event.status = "success"
+        event
+    rescue => detail
+        puts detail.backtrace if Puppet[:trace]
+        event = event(nil)
+        event.status = "failure"
+
+        is = property.is_to_s(is)
+        should = property.should_to_s(should)
+        event.log = property.err "change from #{is} to #{should} failed: #{detail}"
+        event
     end
 
     def forward
@@ -79,7 +80,7 @@ class Puppet::Transaction::Change
 
     # The resource that generated this change.  This is used for handling events,
     # and the proxy resource is used for generated resources, since we can't
-    # send an event to a resource we don't have a direct relationship.  If we
+    # send an event to a resource we don't have a direct relationship with.  If we
     # have a proxy resource, then the events will be considered to be from
     # that resource, rather than us, so the graph resolution will still work.
     def resource
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index a797d9438..abb362123 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -47,37 +47,34 @@ describe Puppet::Transaction do
     describe "when applying changes" do
         before do
             @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
-            @transaction.stubs(:queue_events)
+            @transaction.stubs(:queue_event)
 
             @resource = stub 'resource'
             @property = stub 'property', :is_to_s => "is", :should_to_s => "should"
-            @change = stub 'change', :property => @property, :changed= => nil, :forward => nil, :is => "is", :should => "should"
+
+            @event = stub 'event', :status => "success"
+            @change = stub 'change', :property => @property, :changed= => nil, :forward => @event, :is => "is", :should => "should"
         end
 
         it "should apply each change" do
             c1 = stub 'c1', :property => @property, :changed= => nil
-            c1.expects(:forward)
+            c1.expects(:forward).returns @event
             c2 = stub 'c2', :property => @property, :changed= => nil
-            c2.expects(:forward)
+            c2.expects(:forward).returns @event
 
             @transaction.apply_changes(@resource, [c1, c2])
         end
 
         it "should queue the events from each change" do
-            c1 = stub 'c1', :forward => "e1", :property => @property, :changed= => nil
-            c2 = stub 'c2', :forward => "e2", :property => @property, :changed= => nil
+            c1 = stub 'c1', :forward => stub("event1", :status => "success"), :property => @property, :changed= => nil
+            c2 = stub 'c2', :forward => stub("event2", :status => "success"), :property => @property, :changed= => nil
 
-            @transaction.expects(:queue_events).with(["e1"])
-            @transaction.expects(:queue_events).with(["e2"])
+            @transaction.expects(:queue_event).with(@resource, c1.forward)
+            @transaction.expects(:queue_event).with(@resource, c2.forward)
 
             @transaction.apply_changes(@resource, [c1, c2])
         end
 
-        it "should mark the change as 'changed'" do
-            @change.expects(:changed=).with true
-            @transaction.apply_changes(@resource, [@change])
-        end
-
         it "should store the change in the transaction's change list" do
             @transaction.apply_changes(@resource, [@change])
             @transaction.changes.should include(@change)
@@ -90,22 +87,17 @@ describe Puppet::Transaction do
 
         describe "and a change fails" do
             before do
-                @change.expects(:forward).raises "a failure"
-                @change.property.stubs(:err)
+                @event.stubs(:status).returns "failure"
             end
 
-            it "should rescue the exception" do
-                lambda { @transaction.apply_changes(@resource, [@change]) }.should_not raise_error
-            end
-
-            it "should log the failure with the change's property" do
-                @change.property.expects(:err)
+            it "should increment the failures" do
                 @transaction.apply_changes(@resource, [@change])
+                @transaction.should be_any_failed
             end
 
-            it "should increment the failures" do
+            it "should queue the event" do
+                @transaction.expects(:queue_event).with(@resource, @event)
                 @transaction.apply_changes(@resource, [@change])
-                @transaction.should be_any_failed
             end
         end
     end
@@ -114,38 +106,30 @@ describe Puppet::Transaction do
         before do
             @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
 
-            @graph = stub 'graph', :matching_edges => []
+            @resource = stub("resource", :self_refresh? => false, :deleting => false)
+
+            @graph = stub 'graph', :matching_edges => [], :resource => @resource
             @transaction.stubs(:relationship_graph).returns @graph
 
-            @resource = stub("resource", :self_refresh? => false, :deleting => false)
             @event = Puppet::Transaction::Event.new(:foo, @resource)
         end
 
         it "should store each event in its event list" do
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
 
             @transaction.events.should include(@event)
         end
 
-        it "should use the resource from the first edge as the source of all of the events" do
-            event1 = Puppet::Transaction::Event.new(:foo, @resource)
-            event2 = Puppet::Transaction::Event.new(:foo, stub('resource2', :self_refresh? => false, :deleting? => false))
-
-            @graph.expects(:matching_edges).with { |events, resource| resource == event1.resource }.returns []
-
-            @transaction.queue_events(event1, event2)
-        end
-
         it "should queue events for the target and callback of any matching edges" do
             edge1 = stub("edge1", :callback => :c1, :source => stub("s1"), :target => stub("t1", :c1 => nil))
             edge2 = stub("edge2", :callback => :c2, :source => stub("s2"), :target => stub("t2", :c2 => nil))
 
             @graph.expects(:matching_edges).with { |events, resource| events == [@event] }.returns [edge1, edge2]
 
-            @transaction.expects(:queue_events_for_resource).with(@resource, edge1.target, edge1.callback, [@event])
-            @transaction.expects(:queue_events_for_resource).with(@resource, edge2.target, edge2.callback, [@event])
+            @transaction.expects(:queue_event_for_resource).with(@resource, edge1.target, edge1.callback, @event)
+            @transaction.expects(:queue_event_for_resource).with(@resource, edge2.target, edge2.callback, @event)
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
 
         it "should queue events for the changed resource if the resource is self-refreshing and not being deleted" do
@@ -153,9 +137,9 @@ describe Puppet::Transaction do
 
             @resource.expects(:self_refresh?).returns true
             @resource.expects(:deleting?).returns false
-            @transaction.expects(:queue_events_for_resource).with(@resource, @resource, :refresh, [@event])
+            @transaction.expects(:queue_event_for_resource).with(@resource, @resource, :refresh, @event)
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
 
         it "should not queue events for the changed resource if the resource is not self-refreshing" do
@@ -163,9 +147,9 @@ describe Puppet::Transaction do
 
             @resource.expects(:self_refresh?).returns false
             @resource.stubs(:deleting?).returns false
-            @transaction.expects(:queue_events_for_resource).never
+            @transaction.expects(:queue_event_for_resource).never
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
 
         it "should not queue events for the changed resource if the resource is being deleted" do
@@ -173,9 +157,9 @@ describe Puppet::Transaction do
 
             @resource.expects(:self_refresh?).returns true
             @resource.expects(:deleting?).returns true
-            @transaction.expects(:queue_events_for_resource).never
+            @transaction.expects(:queue_event_for_resource).never
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
 
         it "should ignore edges that don't have a callback" do
@@ -183,9 +167,9 @@ describe Puppet::Transaction do
 
             @graph.expects(:matching_edges).returns [edge1]
 
-            @transaction.expects(:queue_events_for_resource).never
+            @transaction.expects(:queue_event_for_resource).never
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
 
         it "should ignore targets that don't respond to the callback" do
@@ -193,9 +177,9 @@ describe Puppet::Transaction do
 
             @graph.expects(:matching_edges).returns [edge1]
 
-            @transaction.expects(:queue_events_for_resource).never
+            @transaction.expects(:queue_event_for_resource).never
 
-            @transaction.queue_events(@event)
+            @transaction.queue_event(@resource, @event)
         end
     end
 
@@ -212,7 +196,7 @@ describe Puppet::Transaction do
             target = stub("target")
 
             2.times do |i|
-                @transaction.queue_events_for_resource(stub("source", :info => nil), target, "callback#{i}", ["event#{i}"])
+                @transaction.queue_event_for_resource(stub("source", :info => nil), target, "callback#{i}", ["event#{i}"])
             end
 
             @transaction.queued_events(target) { |callback, events| }
@@ -223,7 +207,7 @@ describe Puppet::Transaction do
             source = stub 'source'
             source.expects(:info)
 
-            @transaction.queue_events_for_resource(source, target, "callback", ["event"])
+            @transaction.queue_event_for_resource(source, target, "callback", ["event"])
 
             @transaction.queued_events(target) { |callback, events| }
         end
@@ -232,7 +216,7 @@ describe Puppet::Transaction do
     describe "when processing events for a given resource" do
         before do
             @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new)
-            @transaction.stubs(:queue_events)
+            @transaction.stubs(:queue_event)
 
             @resource = stub 'resource', :notice => nil
             @event = Puppet::Transaction::Event.new(:event, @resource)
@@ -262,8 +246,8 @@ describe Puppet::Transaction do
 
             @resource.stubs(:callback1)
 
-            @transaction.expects(:queue_events).with do |event|
-                event.name == :restarted and event.resource == @resource
+            @transaction.expects(:queue_event).with do |resource, event|
+                event.name == :restarted and resource == @resource
             end
 
             @transaction.process_events(@resource)
@@ -298,7 +282,7 @@ describe Puppet::Transaction do
             end
 
             it "should queue a new noop event" do
-                @transaction.expects(:queue_events).with { |event| event.name == :noop and event.resource == @resource }
+                @transaction.expects(:queue_event).with { |resource, event| event.name == :noop and resource == @resource }
 
                 @transaction.process_events(@resource)
             end
@@ -324,7 +308,7 @@ describe Puppet::Transaction do
             end
 
             it "should not queue a 'restarted' event" do
-                @transaction.expects(:queue_events).never
+                @transaction.expects(:queue_event).never
                 @transaction.process_events(@resource)
             end
 
diff --git a/spec/unit/transaction/change.rb b/spec/unit/transaction/change.rb
index 9e0cedc5c..b42f0691b 100755
--- a/spec/unit/transaction/change.rb
+++ b/spec/unit/transaction/change.rb
@@ -28,10 +28,6 @@ describe Puppet::Transaction::Change do
             # Yay rspec :)
             Change.new(@property, "value").should.should == @property.should
         end
-
-        it "should set its path to the path of the property plus 'change'" do
-            Change.new(@property, "value").path.should == [@property.path, "change"]
-        end
     end
 
     describe "when an instance" do
@@ -55,33 +51,65 @@ describe Puppet::Transaction::Change do
             @change.resource.should == :myresource
         end
 
-        it "should have a method for marking that it's been execution" do
-            @change.changed = true
-            @change.changed?.should be_true
-        end
-
         describe "and creating an event" do
             before do
-                @property.stubs(:resource).returns "myresource"
+                @resource = stub 'resource', :ref => "My[resource]"
+                @property.stubs(:resource).returns @resource
+                @property.stubs(:name).returns :myprop
+            end
+
+            it "should set the event name to the provided name" do
+                @change.event(:foo).name.should == :foo
+            end
+
+            it "should use the property's default event if the event name is nil" do
+                @property.expects(:default_event_name).with(@change.should).returns :myevent
+                @change.event(nil).name.should == :myevent
             end
 
             it "should produce a warning if the event name is not a symbol" do
                 @property.expects(:warning)
-                @property.stubs(:event).returns :myevent
+                @property.stubs(:default_event_name).returns :myevent
                 @change.event("a string")
             end
 
             it "should use the property to generate the event name if the provided name is not a symbol" do
                 @property.stubs(:warning)
-                @property.expects(:event).with(@change.should).returns :myevent
+                @property.expects(:default_event_name).with(@change.should).returns :myevent
 
-                Puppet::Transaction::Event.expects(:new).with { |name, source| name == :myevent }
+                @change.event("a string").name.should == :myevent
+            end
 
-                @change.event("a string")
+            it "should set the resource to the resource reference" do
+                @change.resource.expects(:ref).returns "Foo[bar]"
+                @change.event(:foo).resource.should == "Foo[bar]"
+            end
+
+            it "should set the property to the property name" do
+                @change.property.expects(:name).returns :myprop
+                @change.event(:foo).property.should == :myprop
+            end
+
+            it "should set 'previous_value' from the change's 'is'" do
+                @change.event(:foo).previous_value.should == @change.is
+            end
+
+            it "should set 'desired_value' from the change's 'should'" do
+                @change.event(:foo).desired_value.should == @change.should
             end
         end
 
         describe "and executing" do
+            before do
+                @event = Puppet::Transaction::Event.new(:myevent)
+                @change.stubs(:noop?).returns false
+                @change.stubs(:event).returns @event
+
+                @property.stub_everything
+                @property.stubs(:resource).returns "myresource"
+                @property.stubs(:name).returns :myprop
+            end
+
             describe "in noop mode" do
                 before { @change.stubs(:noop?).returns true }
 
@@ -99,56 +127,64 @@ describe Puppet::Transaction::Change do
 
                     @change.expects(:event).with(:noop).returns :noop_event
 
-                    @change.forward.should == [:noop_event]
+                    @change.forward.should == :noop_event
                 end
             end
 
-            describe "without noop" do
-                before do
-                    @change.stubs(:noop?).returns false
-                    @property.stub_everything
-                    @property.stubs(:resource).returns "myresource"
-                    @property.stubs(:name).returns :myprop
-                end
+            it "should sync the property" do
+                @property.expects(:sync)
 
-                it "should sync the property" do
-                    @property.expects(:sync)
+                @change.forward
+            end
 
-                    @change.forward
-                end
+            it "should return the default event if syncing the property returns nil" do
+                @property.stubs(:sync).returns nil
 
-                it "should return the default event if syncing the property returns nil" do
-                    @property.stubs(:sync).returns nil
+                @change.expects(:event).with(nil).returns @event
 
-                    @change.expects(:event).with(:myprop_changed).returns :myevent
+                @change.forward.should == @event
+            end
 
-                    @change.forward.should == [:myevent]
-                end
+            it "should return the default event if syncing the property returns an empty array" do
+                @property.stubs(:sync).returns []
 
-                it "should return the default event if syncing the property returns an empty array" do
-                    @property.stubs(:sync).returns []
+                @change.expects(:event).with(nil).returns @event
 
-                    @change.expects(:event).with(:myprop_changed).returns :myevent
+                @change.forward.should == @event
+            end
 
-                    @change.forward.should == [:myevent]
-                end
+            it "should log the change" do
+                @property.expects(:sync).returns [:one]
 
-                it "should log the change" do
-                    @property.expects(:sync).returns [:one]
+                @property.expects(:notice).returns "my log"
 
-                    @property.expects(:log)
-                    @property.expects(:change_to_s)
+                @change.forward
+            end
 
-                    @change.forward
-                end
+            it "should set the event's log to the log" do
+                @property.expects(:notice).returns "my log"
+                @change.forward.log.should == "my log"
+            end
+
+            it "should set the event's status to 'success'" do
+                @change.forward.status.should == "success"
+            end
 
-                it "should return an array of events" do
-                    @property.expects(:sync).returns [:one, :two]
+            describe "and the change fails" do
+                before { @property.expects(:sync).raises "an exception" }
 
-                    @change.expects(:event).with(:one).returns :uno
-                    @change.expects(:event).with(:two).returns :dos
+                it "should catch the exception and log the err" do
+                    @property.expects(:err)
+                    lambda { @change.forward }.should_not raise_error
+                end
 
-                    @change.forward.should == [:uno, :dos]
+                it "should mark the event status as 'failure'" do
+                    @change.forward.status.should == "failure"
+                end
+
+                it "should set the event log to a failure log" do
+                    @property.expects(:err).returns "my failure"
+                    @change.forward.log.should == "my failure"
                 end
             end
 
@@ -177,9 +213,9 @@ describe Puppet::Transaction::Change do
                     @change.backward
                 end
 
-                it "should execute" do
-                    @change.expects(:go)
-                    @change.backward
+                it "should execute and return the resulting event" do
+                    @change.expects(:go).returns :myevent
+                    @change.backward.should == :myevent
                 end
             end
         end
-- 
cgit