Refactoring the Change/Event/Property interface

This gives all logging responsibility to the event, which
can now produce logs identical to those produced directly by
the property.

At this point, the events are entirely supersets of the logs.

6 files changed, 108 insertions, 21 deletions

diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 9d50dcf6a..ad8ea623f 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -157,7 +157,7 @@ class Puppet::Property < Puppet::Parameter
 
     # Return a modified form of the resource event.
     def event
-        resource.event :name => event_name, :desired_value => should, :property => name
+        resource.event :name => event_name, :desired_value => should, :property => name, :source_description => path
     end
 
     attr_reader :shadow
diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index 49569d6e3..f8e2a7a82 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -40,8 +40,9 @@ class Puppet::Transaction::Change
         property.sync
 
         result = event()
-        result.log = property.notice property.change_to_s(is, should)
+        result.message = property.change_to_s(is, should)
         result.status = "success"
+        result.send_log
         result
     rescue => detail
         puts detail.backtrace if Puppet[:trace]
@@ -50,7 +51,8 @@ class Puppet::Transaction::Change
 
         is = property.is_to_s(is)
         should = property.should_to_s(should)
-        result.log = property.err "change from #{is} to #{should} failed: #{detail}"
+        result.message = "change from #{is} to #{should} failed: #{detail}"
+        result.send_log
         result
     end
 
@@ -80,8 +82,9 @@ class Puppet::Transaction::Change
 
     def noop_event
         result = event
-        result.log = property.log "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)"
+        result.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)"
         result.status = "noop"
+        result.send_log
         return result
     end
 end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index abd5c8041..41e1f5130 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -1,11 +1,13 @@
 require 'puppet/transaction'
 require 'puppet/util/tagging'
+require 'puppet/util/logging'
 
 # A simple struct for storing what happens on the system.
 class Puppet::Transaction::Event 
     include Puppet::Util::Tagging
+    include Puppet::Util::Logging
 
-    ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :status, :log, :node, :version, :file, :line]
+    ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :status, :message, :node, :version, :file, :line, :source_description]
     attr_accessor *ATTRIBUTES
     attr_writer :tags
     attr_accessor :time
@@ -19,12 +21,31 @@ class Puppet::Transaction::Event
         @time = Time.now
     end
 
+    def property=(prop)
+        @property = prop.to_s
+    end
+
+    def resource=(res)
+        @resource = res.to_s
+    end
+
+    def send_log
+        super(status == "failure" ? :err : :notice, message)
+    end
+
     def status=(value)
         raise ArgumentError, "Event status can only be #{EVENT_STATUSES.join(', ')}" unless EVENT_STATUSES.include?(value)
         @status = value
     end
 
     def to_s
-        log
+        message
+    end
+
+    private
+
+    # Used by the Logging module
+    def log_source
+        source_description || property || resource
     end
 end
diff --git a/spec/unit/property.rb b/spec/unit/property.rb
index 5bc29a61a..eba5c3d83 100755
--- a/spec/unit/property.rb
+++ b/spec/unit/property.rb
@@ -136,6 +136,11 @@ describe Puppet::Property do
             @instance.stubs(:should).returns "foo"
             @instance.event.desired_value.should == "foo"
         end
+
+        it "should provide its path as the source description" do
+            @instance.stubs(:path).returns "/my/param"
+            @instance.event.source_description.should == "/my/param"
+        end
     end
 
     describe "when shadowing metaparameters" do
diff --git a/spec/unit/transaction/change.rb b/spec/unit/transaction/change.rb
index 4c22c8327..be1267bf6 100755
--- a/spec/unit/transaction/change.rb
+++ b/spec/unit/transaction/change.rb
@@ -77,6 +77,7 @@ describe Puppet::Transaction::Change do
         describe "and executing" do
             before do
                 @event = Puppet::Transaction::Event.new(:myevent)
+                @event.stubs(:send_log)
                 @change.stubs(:noop?).returns false
                 @property.stubs(:event).returns @event
 
@@ -91,9 +92,8 @@ describe Puppet::Transaction::Change do
                 it "should log that it is in noop" do
                     @property.expects(:is_to_s)
                     @property.expects(:should_to_s)
-                    @property.expects(:log).returns "my log"
 
-                    @event.expects(:log=).with("my log")
+                    @event.expects(:message=).with { |msg| msg.include?("should be") }
 
                     @change.forward
                 end
@@ -132,14 +132,14 @@ describe Puppet::Transaction::Change do
             it "should log the change" do
                 @property.expects(:sync).returns [:one]
 
-                @property.expects(:notice).returns "my log"
+                @event.expects(:send_log)
 
                 @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"
+            it "should set the event's message to the change log" do
+                @property.expects(:change_to_s).returns "my change"
+                @change.forward.message.should == "my change"
             end
 
             it "should set the event's status to 'success'" do
@@ -150,7 +150,7 @@ describe Puppet::Transaction::Change do
                 before { @property.expects(:sync).raises "an exception" }
 
                 it "should catch the exception and log the err" do
-                    @property.expects(:err)
+                    @event.expects(:send_log)
                     lambda { @change.forward }.should_not raise_error
                 end
 
@@ -159,8 +159,7 @@ describe Puppet::Transaction::Change do
                 end
 
                 it "should set the event log to a failure log" do
-                    @property.expects(:err).returns "my failure"
-                    @change.forward.log.should == "my failure"
+                    @change.forward.message.should be_include("failed")
                 end
             end
 
diff --git a/spec/unit/transaction/event.rb b/spec/unit/transaction/event.rb
index 7bdd0898e..07470b2b9 100755
--- a/spec/unit/transaction/event.rb
+++ b/spec/unit/transaction/event.rb
@@ -5,7 +5,7 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/transaction/event'
 
 describe Puppet::Transaction::Event do
-    [:log, :previous_value, :desired_value, :property, :resource, :name, :log, :node, :version, :file, :line, :tags].each do |attr|
+    [:previous_value, :desired_value, :property, :resource, :name, :message, :node, :version, :file, :line, :tags].each do |attr|
         it "should support #{attr}" do
             event = Puppet::Transaction::Event.new
             event.send(attr.to_s + "=", "foo")
@@ -13,10 +13,18 @@ describe Puppet::Transaction::Event do
         end
     end
 
-    it "should produce the log when converted to a string" do
+    it "should always convert the property to a string" do
+        Puppet::Transaction::Event.new(:property => :foo).property.should == "foo"
+    end
+
+    it "should always convert the resource to a string" do
+        Puppet::Transaction::Event.new(:resource => :foo).resource.should == "foo"
+    end
+
+    it "should produce the message when converted to a string" do
         event = Puppet::Transaction::Event.new
-        event.expects(:log).returns "my log"
-        event.to_s.should == "my log"
+        event.expects(:message).returns "my message"
+        event.to_s.should == "my message"
     end
 
     it "should support 'status'" do
@@ -34,9 +42,60 @@ describe Puppet::Transaction::Event do
         Puppet::Transaction::Event.ancestors.should include(Puppet::Util::Tagging)
     end
 
-    it "should be able to send logs"
-
     it "should create a timestamp at its creation time" do
         Puppet::Transaction::Event.new.time.should be_instance_of(Time)
     end
+
+    describe "when sending logs" do
+        before do
+            Puppet::Util::Log.stubs(:new)
+        end
+
+        it "should set the level to 'notice' if the event status is 'success'" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:level] == :notice }
+            Puppet::Transaction::Event.new(:status => "success").send_log
+        end
+
+        it "should set the level to 'notice' if the event status is 'noop'" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:level] == :notice }
+            Puppet::Transaction::Event.new(:status => "noop").send_log
+        end
+
+        it "should set the level to 'err' if the event status is 'failure'" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:level] == :err }
+            Puppet::Transaction::Event.new(:status => "failure").send_log
+        end
+
+        it "should set the 'message' to the event log" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:message] == "my message" }
+            Puppet::Transaction::Event.new(:message => "my message").send_log
+        end
+
+        it "should set the tags to the event tags" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:tags] == %w{one two} }
+            Puppet::Transaction::Event.new(:tags => %w{one two}).send_log
+        end
+
+        [:file, :line, :version].each do |attr|
+            it "should pass the #{attr}" do
+                Puppet::Util::Log.expects(:new).with { |args| args[attr] == "my val" }
+                Puppet::Transaction::Event.new(attr => "my val").send_log
+            end
+        end
+
+        it "should use the source description as the source if one is set" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:source] == "/my/param" }
+            Puppet::Transaction::Event.new(:source_description => "/my/param", :resource => "Foo[bar]", :property => "foo").send_log
+        end
+
+        it "should use the property as the source if one is available and no source description is set" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:source] == "foo" }
+            Puppet::Transaction::Event.new(:resource => "Foo[bar]", :property => "foo").send_log
+        end
+
+        it "should use the property as the source if one is available and no property or source description is set" do
+            Puppet::Util::Log.expects(:new).with { |args| args[:source] == "Foo[bar]" }
+            Puppet::Transaction::Event.new(:resource => "Foo[bar]").send_log
+        end
+    end
 end