14 files changed, 308 insertions, 181 deletions

diff --git a/lib/puppet/indirector/code/report.rb b/lib/puppet/indirector/code/report.rb
new file mode 100644
index 000000000..a3c3a6df5
--- /dev/null
+++ b/lib/puppet/indirector/code/report.rb
@@ -0,0 +1,49 @@
+require 'puppet/indirector/code'
+require 'puppet/reports'
+
+class Puppet::Indirector::Code::Report < Puppet::Indirector::Code
+    desc "Puppet's report processor.  Processes the report with each of
+        the report types listed in the 'reports' setting."
+
+    def initialize
+        Puppet.settings.use(:reporting, :metrics)
+    end
+
+    def save(report)
+        process(report)
+    end
+
+    private
+
+    # Process the report with each of the configured report types.
+    # LAK:NOTE This isn't necessarily the best design, but it's backward
+    # compatible and that's good enough for now.
+    def process(report)
+        return if Puppet[:reports] == "none"
+
+        reports().each do |name|
+            if mod = Puppet::Reports.report(name)
+                # We have to use a dup because we're including a module in the
+                # report.
+                newrep = report.dup
+                begin
+                    newrep.extend(mod)
+                    newrep.process
+                rescue => detail
+                    if Puppet[:trace]
+                        puts detail.backtrace
+                    end
+                    Puppet.err "Report %s failed: %s" %
+                        [name, detail]
+                end
+            else
+                Puppet.warning "No report named '%s'" % name
+            end
+        end
+    end
+
+    # Handle the parsing of the reports attribute.
+    def reports
+        Puppet[:reports].gsub(/(^\s+)|(\s+$)/, '').split(/\s*,\s*/)
+    end
+end
diff --git a/lib/puppet/network/handler/report.rb b/lib/puppet/network/handler/report.rb
index e202d4e2a..12abc9b15 100755
--- a/lib/puppet/network/handler/report.rb
+++ b/lib/puppet/network/handler/report.rb
@@ -1,78 +1,24 @@
 require 'puppet/util/instance_loader'
+require 'puppet/reports'
 
 # A simple server for triggering a new run on a Puppet client.
 class Puppet::Network::Handler
     class Report < Handler
         desc "Accepts a Puppet transaction report and processes it."
 
-        extend Puppet::Util::ClassGen
-        extend Puppet::Util::InstanceLoader
-
-        module ReportBase
-            include Puppet::Util::Docs
-            attr_writer :useyaml
-
-            def useyaml?
-                if defined? @useyaml
-                    @useyaml
-                else
-                    false
-                end
-            end
-        end
-
         @interface = XMLRPC::Service::Interface.new("puppetreports") { |iface|
             iface.add_method("string report(array)")
         }
 
-        # Set up autoloading and retrieving of reports.
-        instance_load :report, 'puppet/reports'
-
-        class << self
-            attr_reader :hooks
-        end
-
         # Add a new report type.
         def self.newreport(name, options = {}, &block)
-            name = symbolize(name)
-
-            mod = genmodule(name, :extend => ReportBase, :hash => instance_hash(:report), :block => block)
-
-            if options[:useyaml]
-                mod.useyaml = true
-            end
-
-            mod.send(:define_method, :report_name) do
-                name
-            end
-        end
-
-        # Collect the docs for all of our reports.
-        def self.reportdocs
-            docs = ""
-
-            # Use this method so they all get loaded
-            instance_loader(:report).loadall
-            loaded_instances(:report).sort { |a,b| a.to_s <=> b.to_s }.each do |name|
-                mod = self.report(name)
-                docs += "%s\n%s\n" % [name, "-" * name.to_s.length]
-
-                docs += Puppet::Util::Docs.scrub(mod.doc) + "\n\n"
-            end
-
-            docs
-        end
-
-        # List each of the reports.
-        def self.reports
-            instance_loader(:report).loadall
-            loaded_instances(:report)
+            Puppet.warning "The interface for registering report types has changed; use Puppet::Reports.register_report for report type %s" % name
+            Puppet::Reports.register_report(name, options, &block)
         end
 
         def initialize(*args)
             super
-            Puppet.settings.use(:reporting)
-            Puppet.settings.use(:metrics)
+            Puppet.settings.use(:reporting, :metrics)
         end
 
         # Accept a report from a client.
@@ -111,17 +57,13 @@ class Puppet::Network::Handler
             client = report.host
 
             reports().each do |name|
-                if mod = self.class.report(name)
+                if mod = Puppet::Reports.report(name)
                     # We have to use a dup because we're including a module in the
                     # report.
                     newrep = report.dup
                     begin
                         newrep.extend(mod)
-                        if mod.useyaml?
-                            newrep.process(yaml)
-                        else
-                            newrep.process
-                        end
+                        newrep.process
                     rescue => detail
                         if Puppet[:trace]
                             puts detail.backtrace
diff --git a/lib/puppet/reports.rb b/lib/puppet/reports.rb
new file mode 100755
index 000000000..df992d46c
--- /dev/null
+++ b/lib/puppet/reports.rb
@@ -0,0 +1,51 @@
+require 'puppet/util/instance_loader'
+
+# A simple mechanism for loading and returning reports.
+class Puppet::Reports
+    extend Puppet::Util::ClassGen
+    extend Puppet::Util::InstanceLoader
+
+    # Set up autoloading and retrieving of reports.
+    instance_load :report, 'puppet/reports'
+
+    class << self
+        attr_reader :hooks
+    end
+
+    # Add a new report type.
+    def self.register_report(name, options = {}, &block)
+        name = symbolize(name)
+
+        mod = genmodule(name, :extend => Puppet::Util::Docs, :hash => instance_hash(:report), :block => block)
+
+        if options[:useyaml]
+            mod.useyaml = true
+        end
+
+        mod.send(:define_method, :report_name) do
+            name
+        end
+    end
+
+    # Collect the docs for all of our reports.
+    def self.reportdocs
+        docs = ""
+
+        # Use this method so they all get loaded
+        instance_loader(:report).loadall
+        loaded_instances(:report).sort { |a,b| a.to_s <=> b.to_s }.each do |name|
+            mod = self.report(name)
+            docs += "%s\n%s\n" % [name, "-" * name.to_s.length]
+
+            docs += Puppet::Util::Docs.scrub(mod.doc) + "\n\n"
+        end
+
+        docs
+    end
+
+    # List each of the reports.
+    def self.reports
+        instance_loader(:report).loadall
+        loaded_instances(:report)
+    end
+end
diff --git a/lib/puppet/reports/log.rb b/lib/puppet/reports/log.rb
index 8f7b95f8b..8dfeedb07 100644
--- a/lib/puppet/reports/log.rb
+++ b/lib/puppet/reports/log.rb
@@ -1,6 +1,6 @@
-require 'puppet'
+require 'puppet/reports'
 
-Puppet::Network::Handler.report.newreport(:log) do
+Puppet::Reports.register_report(:log) do
     desc "Send all received logs to the local log destinations.  Usually
         the log destination is syslog."
 
diff --git a/lib/puppet/reports/rrdgraph.rb b/lib/puppet/reports/rrdgraph.rb
index 9ab1f4b17..2611f0369 100644
--- a/lib/puppet/reports/rrdgraph.rb
+++ b/lib/puppet/reports/rrdgraph.rb
@@ -1,6 +1,4 @@
-require 'puppet'
-
-Puppet::Network::Handler.report.newreport(:rrdgraph) do
+Puppet::Reports.register_report(:rrdgraph) do
     desc "Graph all available data about hosts using the RRD library.  You
         must have the Ruby RRDtool library installed to use this report, which
         you can get from `the RubyRRDTool RubyForge page`_.  This package requires
diff --git a/lib/puppet/reports/store.rb b/lib/puppet/reports/store.rb
index 0c3e86f37..dfc992820 100644
--- a/lib/puppet/reports/store.rb
+++ b/lib/puppet/reports/store.rb
@@ -1,6 +1,6 @@
 require 'puppet'
 
-Puppet::Network::Handler.report.newreport(:store, :useyaml => true) do
+Puppet::Reports.register_report(:store) do
     Puppet.settings.use(:reporting)
 
     desc "Store the yaml report on disk.  Each host sends its report as a YAML dump
@@ -24,7 +24,7 @@ Puppet::Network::Handler.report.newreport(:store, :useyaml => true) do
         config.use("reportclient-#{client}")
     end
 
-    def process(yaml)
+    def process
         # We don't want any tracking back in the fs.  Unlikely, but there
         # you go.
         client = self.host.gsub("..",".")
@@ -46,7 +46,7 @@ Puppet::Network::Handler.report.newreport(:store, :useyaml => true) do
 
         begin
             File.open(file, "w", 0640) do |f|
-                f.print yaml
+                f.print to_yaml
             end
         rescue => detail
             if Puppet[:trace]
diff --git a/lib/puppet/reports/tagmail.rb b/lib/puppet/reports/tagmail.rb
index fd9126a1a..a2c973f8f 100644
--- a/lib/puppet/reports/tagmail.rb
+++ b/lib/puppet/reports/tagmail.rb
@@ -3,7 +3,7 @@ require 'pp'
 
 require 'net/smtp'
 
-Puppet::Network::Handler.report.newreport(:tagmail) do
+Puppet::Reports.register_report(:tagmail) do
     desc "This report sends specific log messages to specific email addresses
         based on the tags in the log messages.  See the
         `UsingTags tag documentation`:trac: for more information
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index 19dd1b629..f73c6a9fb 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -1,10 +1,15 @@
 require 'puppet'
+require 'puppet/indirector'
 
 # A class for reporting what happens on each client.  Reports consist of
 # two types of data:  Logs and Metrics.  Logs are the output that each
 # change produces, and Metrics are all of the numerical data involved
 # in the transaction.
 class Puppet::Transaction::Report
+    extend Puppet::Indirector
+
+    indirects :report, :terminus_class => :code
+
     attr_accessor :logs, :metrics, :time, :host
     
     def <<(msg)
diff --git a/spec/integration/reports.rb b/spec/integration/reports.rb
new file mode 100755
index 000000000..7351c3da1
--- /dev/null
+++ b/spec/integration/reports.rb
@@ -0,0 +1,14 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-10-12.
+#  Copyright (c) 2007. All rights reserved.
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+require 'puppet/reports'
+
+describe Puppet::Reports, " when using report types" do
+    it "should load report types as modules" do
+        Puppet::Reports.report(:store).should be_instance_of(Module)
+    end
+end
diff --git a/spec/unit/indirector/code/report.rb b/spec/unit/indirector/code/report.rb
new file mode 100755
index 000000000..a27eaa297
--- /dev/null
+++ b/spec/unit/indirector/code/report.rb
@@ -0,0 +1,73 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-9-23.
+#  Copyright (c) 2007. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../../spec_helper'
+
+require 'puppet/indirector/code/report'
+
+describe Puppet::Indirector::Code::Report do
+    it "should provide a method for saving reports" do
+        Puppet::Indirector::Code::Report.new.should respond_to(:save)
+    end
+end
+
+
+describe Puppet::Indirector::Code::Report, " when saving a report" do
+    before do
+        Puppet.settings.stubs(:use)
+        @reporter = Puppet::Indirector::Code::Report.new
+    end
+
+    it "should not process the report if reports are set to 'none'" do
+        Puppet::Reports.expects(:report).never
+        Puppet.settings.expects(:value).with(:reports).returns("none")
+
+        @reporter.save(:whatever)
+    end
+
+    it "should process the report with each configured report type" do
+        Puppet.settings.stubs(:value).with(:reports).returns("one,two")
+        @reporter.send(:reports).should == %w{one two}
+    end
+end
+
+describe Puppet::Indirector::Code::Report, " when processing a report" do
+    before do
+        Puppet.settings.stubs(:value).with(:reports).returns("one")
+        Puppet.settings.stubs(:use)
+        @reporter = Puppet::Indirector::Code::Report.new
+
+        @report_type = mock 'one'
+        @dup_report = mock 'dupe report'
+        @dup_report.stubs(:process)
+        @report = mock 'report'
+        @report.expects(:dup).returns(@dup_report)
+        Puppet::Reports.expects(:report).with("one").returns(@report_type)
+
+        @dup_report.expects(:extend).with(@report_type)
+    end
+
+    # LAK:NOTE This is stupid, because the code is so short it doesn't
+    # make sense to split it out, which means I just do the same test
+    # three times so the spec looks right.
+    it "should process a duplicate of the report, not the original" do
+        @reporter.save(@report)
+    end
+
+    it "should extend the report with the report type's module" do
+        @reporter.save(@report)
+    end
+
+    it "should call the report type's :process method" do
+        @dup_report.expects(:process)
+        @reporter.save(@report)
+    end
+
+    it "should not raise exceptions" do
+        Puppet.settings.stubs(:value).with(:trace).returns(false)
+        @dup_report.expects(:process).raises(ArgumentError)
+        proc { @reporter.save(@report) }.should_not raise_error
+    end
+end
diff --git a/spec/unit/reports.rb b/spec/unit/reports.rb
new file mode 100755
index 000000000..f12f0d717
--- /dev/null
+++ b/spec/unit/reports.rb
@@ -0,0 +1,61 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../spec_helper'
+
+require 'puppet/reports'
+
+describe Puppet::Reports do
+    it "should instance-load report types" do
+        Puppet::Reports.instance_loader(:report).should be_instance_of(Puppet::Util::Autoload)
+    end
+
+    it "should have a method for registering report types" do
+        Puppet::Reports.should respond_to(:register_report)
+    end
+
+    it "should have a method for retrieving report types by name" do
+        Puppet::Reports.should respond_to(:report)
+    end
+
+    it "should provide a method for returning documentation for all reports" do
+        Puppet::Reports.expects(:loaded_instances).with(:report).returns([:one, :two])
+        one = mock 'one', :doc => "onedoc"
+        two = mock 'two', :doc => "twodoc"
+        Puppet::Reports.expects(:report).with(:one).returns(one)
+        Puppet::Reports.expects(:report).with(:two).returns(two)
+
+        doc = Puppet::Reports.reportdocs
+        doc.include?("onedoc").should be_true
+        doc.include?("twodoc").should be_true
+    end
+end
+
+
+describe Puppet::Reports, " when loading report types" do
+    it "should use the instance loader to retrieve report types" do
+        Puppet::Reports.expects(:loaded_instance).with(:report, :myreporttype)
+        Puppet::Reports.report(:myreporttype)
+    end
+end
+
+describe Puppet::Reports, " when registering report types" do
+    it "should evaluate the supplied block as code for a module" do
+        Puppet::Reports.expects(:genmodule).returns(Module.new)
+        Puppet::Reports.register_report(:testing) { }
+    end
+
+    it "should extend the report type with the Puppet::Util::Docs module" do
+        mod = stub 'module', :define_method => true
+
+        Puppet::Reports.expects(:genmodule).with { |name, options, block| options[:extend] == Puppet::Util::Docs }.returns(mod)
+        Puppet::Reports.register_report(:testing) { }
+    end
+
+    it "should define a :report_name method in the module that returns the name of the report" do
+        mod = mock 'module'
+        mod.expects(:define_method).with(:report_name)
+
+        Puppet::Reports.expects(:genmodule).returns(mod)
+        Puppet::Reports.register_report(:testing) { }
+    end
+end
diff --git a/spec/unit/transaction/report.rb b/spec/unit/transaction/report.rb
new file mode 100755
index 000000000..ce8c4038d
--- /dev/null
+++ b/spec/unit/transaction/report.rb
@@ -0,0 +1,34 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-10-12.
+#  Copyright (c) 2007. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/transaction/report'
+
+describe Puppet::Transaction::Report, " when being indirect" do
+    it "should redirect :find to the indirection" do
+        @indirection = mock 'indirection'
+        Puppet::Transaction::Report.stubs(:indirection).returns(@indirection)
+        @indirection.expects(:find).with(:report)
+        Puppet::Transaction::Report.find(:report)
+    end
+
+    it "should redirect :save to the indirection" do
+        Facter.stubs(:value).returns("eh")
+        @indirection = mock 'indirection'
+        Puppet::Transaction::Report.stubs(:indirection).returns(@indirection)
+        report = Puppet::Transaction::Report.new
+        @indirection.expects(:save).with(report)
+        report.save
+    end
+
+    it "should default to the 'code' terminus" do
+        Puppet::Transaction::Report.indirection.terminus_class.should == :code
+    end
+
+    after do
+        Puppet::Indirector::Indirection.clear_cache
+    end
+end
diff --git a/test/network/handler/report.rb b/test/network/handler/report.rb
index ac8cf6fac..76326bce6 100755
--- a/test/network/handler/report.rb
+++ b/test/network/handler/report.rb
@@ -31,46 +31,13 @@ class TestReportServer < Test::Unit::TestCase
         client
     end
 
-    def test_report_autoloading
-        # Create a fake report
-        fakedir = tempfile()
-        $: << fakedir
-        cleanup do $:.delete(fakedir) end
-
-        libdir = File.join(fakedir, "puppet", "reports")
-        FileUtils.mkdir_p(libdir)
-
-        $myreportrun = false
-        file = File.join(libdir, "myreport.rb")
-        File.open(file, "w") { |f| f.puts %{
-                Puppet::Network::Handler.report.newreport(:myreport) do
-                    def process(report)
-                        $myreportrun = true
-                        return report
-                    end
-                end
-            }
-        }
-        Puppet[:reports] = "myreport"
-
-        # Create a server
-        server = Puppet::Network::Handler.report.new
-
-        report = nil
-        assert_nothing_raised {
-            report = Puppet::Network::Handler.report.report(:myreport)
-        }
-        assert(report, "Did not get report")
-
-    end
-
     def test_process
         server = Puppet::Network::Handler.report.new
 
         # We have to run multiple reports to make sure there's no conflict
         reports = []
         $run = []
-        5.times do |i|
+        2.times do |i|
             name = "processtest%s" % i
             reports << name
 
@@ -100,35 +67,6 @@ class TestReportServer < Test::Unit::TestCase
         }
     end
 
-    # Make sure reports can specify whether to use yaml or not
-    def test_useyaml
-        server = Puppet::Network::Handler.report.new
-
-        Report.newreport(:yamlyes, :useyaml => true) do
-            def process(report)
-                $yamlyes = :yesyaml
-            end
-        end
-
-        Report.newreport(:yamlno) do
-            def process
-                $yamlno = :noyaml
-            end
-        end
-
-        Puppet[:reports] = "yamlyes, yamlno"
-
-        report = fakereport
-        yaml = YAML.dump(report)
-
-        assert_nothing_raised do
-            server.send(:process, yaml)
-        end
-
-        assert_equal(:noyaml, $yamlno, "YAML was used for non-yaml report")
-        assert_equal(:yesyaml, $yamlyes, "YAML was not used for yaml report")
-    end
-
     def test_reports
         Puppet[:reports] = "myreport"
 
@@ -142,43 +80,4 @@ class TestReportServer < Test::Unit::TestCase
             assert_equal(ary, server.send(:reports))
         end
     end
-
-    def test_newreport
-        name = :newreporttest
-        assert_nothing_raised do
-            Report.newreport(name) do
-                attr_accessor :processed
-
-                def process(report)
-                    @processed = report
-                end
-            end
-        end
-
-        assert(Report.report(name), "Did not get report")
-        assert_instance_of(Module, Report.report(name))
-
-        obj = "yay"
-        obj.extend(Report.report(name))
-
-        assert_nothing_raised do
-            obj.process("yay")
-        end
-
-        assert_equal("yay", obj.processed)
-    end
-
-    # Make sure we get a list of all reports
-    def test_report_list
-        list = nil
-        assert_nothing_raised do
-            list = Puppet::Network::Handler.report.reports
-        end
-
-        [:rrdgraph, :store, :tagmail].each do |name|
-            assert(list.include?(name), "Did not load %s" % name)
-        end
-    end
 end
-
-
diff --git a/test/other/report.rb b/test/other/report.rb
index 8f33a7166..bed510f2f 100755
--- a/test/other/report.rb
+++ b/test/other/report.rb
@@ -3,6 +3,7 @@
 $:.unshift("../lib").unshift("../../lib") if __FILE__ =~ /\.rb$/
 
 require 'puppet'
+require 'puppet/reports'
 require 'puppet/transaction/report'
 require 'puppettest'
 require 'puppettest/reporttesting'
@@ -97,14 +98,14 @@ class TestReports < Test::Unit::TestCase
         }
 
         assert_nothing_raised do
-            report.extend(Puppet::Network::Handler.report.report(:store))
+            report.extend(Puppet::Reports.report(:store))
         end
 
         yaml = YAML.dump(report)
 
         file = nil
         assert_nothing_raised {
-            file = report.process(yaml)
+            file = report.process
         }
 
         assert(FileTest.exists?(file), "report file did not get created")
@@ -119,7 +120,7 @@ class TestReports < Test::Unit::TestCase
         assert(! report.metrics.empty?, "Did not receive any metrics")
 
         assert_nothing_raised do
-            report.extend(Puppet::Network::Handler.report.report(:rrdgraph))
+            report.extend(Puppet::Reports.report(:rrdgraph))
         end
 
         assert_nothing_raised {
@@ -154,7 +155,7 @@ class TestReports < Test::Unit::TestCase
     
     def test_tagmail_parsing
         report = Object.new
-        report.extend(Puppet::Network::Handler.report.report(:tagmail))
+        report.extend(Puppet::Reports.report(:tagmail))
         
         passers = File.join(datadir, "reports", "tagmail_passers.conf")
         assert(FileTest.exists?(passers), "no passers file %s" % passers)
@@ -178,7 +179,7 @@ class TestReports < Test::Unit::TestCase
     
     def test_tagmail_parsing_results
         report = Object.new
-        report.extend(Puppet::Network::Handler.report.report(:tagmail))
+        report.extend(Puppet::Reports.report(:tagmail))
         # Now test a few specific lines to make sure we get the results we want
         {
             "tag: abuse@domain.com" => [%w{abuse@domain.com}, %w{tag}, []],
@@ -206,7 +207,7 @@ class TestReports < Test::Unit::TestCase
         
         list = report.logs.collect { |l| l.to_report }
         
-        report.extend(Puppet::Network::Handler.report.report(:tagmail))
+        report.extend(Puppet::Reports.report(:tagmail))
         
         {
             [%w{abuse@domain.com}, %w{all}, []] => list,