Translating the report handler to an indirected model.

I've provided backward compatibility with the old
handler.

The only terminus type that currently exists for reports
is the 'code' terminus, which is used to process reports
in the style of the old handler.  At some point, we should
likely switch at least some of these report types (e.g., 'store')
to terminus types.

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,