From: Paul Berry <[email protected]>

Now if one of the children of reports fail (events, logs, resource
statuses) while importing, none of the report will import and won't
leave the database polluted.

Paired-with: Matt Robinson

Signed-off-by: Matt Robinson <[email protected]>
---
Local-branch: ticket/next/5543
 app/models/report.rb       |  148 ++++++++++++++++++++++----------------------
 spec/models/report_spec.rb |   11 +++
 2 files changed, 86 insertions(+), 73 deletions(-)

diff --git a/app/models/report.rb b/app/models/report.rb
index fbe65ef..0352a8a 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -86,89 +86,91 @@ class Report < ActiveRecord::Base
   end
 
   def self.create_from_yaml(report_yaml)
-    raw_report = YAML.load(report_yaml)
+    ActiveRecord::Base.transaction do
+      raw_report = YAML.load(report_yaml)
 
-    unless raw_report.is_a? Puppet::Transaction::Report
-      raise ArgumentError, "The supplied report is in invalid format 
'#{raw_report.class}', expected 'Puppet::Transaction::Report'"
-    end
+      unless raw_report.is_a? Puppet::Transaction::Report
+        raise ArgumentError, "The supplied report is in invalid format 
'#{raw_report.class}', expected 'Puppet::Transaction::Report'"
+      end
 
-    raw_report.extend(ReportExtensions)
-
-    report = Report.create!(
-      :time => raw_report.time,
-      :host => raw_report.host,
-      :kind => raw_report.kind
-    )
-
-    total_time = nil
-    raw_report.metrics.each do |metric_category, metrics|
-      metrics.values.each do |name, _, value|
-        total_time = value if metric_category.to_s == "time" and name.to_s == 
"total"
-        report.metrics.create!(
-          :category  => metric_category.to_s,
-          :name      => name.to_s,
-          :value     => value
-        )
+      raw_report.extend(ReportExtensions)
+
+      report = Report.create!(
+        :time => raw_report.time,
+        :host => raw_report.host,
+        :kind => raw_report.kind
+      )
+
+      total_time = nil
+      raw_report.metrics.each do |metric_category, metrics|
+        metrics.values.each do |name, _, value|
+          total_time = value if metric_category.to_s == "time" and name.to_s 
== "total"
+          report.metrics.create!(
+            :category  => metric_category.to_s,
+            :name      => name.to_s,
+            :value     => value
+          )
+        end
       end
-    end
-    unless total_time
-      time_metrics = raw_report.metric_value(:time)
-      if time_metrics
-        total_time = time_metrics.values.sum(&:last) 
-        report.metrics.create!(
-          :category => "time",
-          :name     => "total",
-          :value    => total_time
+      unless total_time
+        time_metrics = raw_report.metric_value(:time)
+        if time_metrics
+          total_time = time_metrics.values.sum(&:last) 
+          report.metrics.create!(
+            :category => "time",
+            :name     => "total",
+            :value    => total_time
+          )
+        end
+      end
+
+      raw_report.resource_statuses.each do |resource,status|
+        resource =~ /^(.+?)\[(.+)\]$/
+        resource_type, title = $1, $2
+        resource_status = report.resource_statuses.create!(
+          :resource_type      => resource_type,
+          :title              => title,
+          :evaluation_time    => status.evaluation_time,
+          :file               => status.file,
+          :line               => status.line,
+          :source_description => status.source_description,
+          :tags               => status.tags,
+          :time               => status.time,
+          :change_count       => status.change_count || 0,
+          :out_of_sync        => status.out_of_sync
         )
+        status.events.each do |event|
+          resource_status.events.create!(
+            :property           => event.property,
+            :previous_value     => event.previous_value,
+            :desired_value      => event.desired_value,
+            :message            => event.message,
+            :name               => event.name.to_s,
+            :source_description => event.source_description,
+            :status             => event.status,
+            :tags               => event.tags,
+            :time               => event.time
+          )
+        end
       end
-    end
 
-    raw_report.resource_statuses.each do |resource,status|
-      resource =~ /^(.+?)\[(.+)\]$/
-      resource_type, title = $1, $2
-      resource_status = report.resource_statuses.create!(
-        :resource_type      => resource_type,
-        :title              => title,
-        :evaluation_time    => status.evaluation_time,
-        :file               => status.file,
-        :line               => status.line,
-        :source_description => status.source_description,
-        :tags               => status.tags,
-        :time               => status.time,
-        :change_count       => status.change_count || 0,
-        :out_of_sync        => status.out_of_sync
-      )
-      status.events.each do |event|
-        resource_status.events.create!(
-          :property           => event.property,
-          :previous_value     => event.previous_value,
-          :desired_value      => event.desired_value,
-          :message            => event.message,
-          :name               => event.name.to_s,
-          :source_description => event.source_description,
-          :status             => event.status,
-          :tags               => event.tags,
-          :time               => event.time
+      raw_report.logs.each do |log|
+        report.logs.create!(
+          :level   => log.level.to_s,
+          :message => log.message,
+          :source  => log.source,
+          :tags    => log.tags,
+          :time    => log.time,
+          :file    => log.file,
+          :line    => log.line
         )
       end
-    end
 
-    raw_report.logs.each do |log|
-      report.logs.create!(
-        :level   => log.level.to_s,
-        :message => log.message,
-        :source  => log.source,
-        :tags    => log.tags,
-        :time    => log.time,
-        :file    => log.file,
-        :line    => log.line
-      )
+      report.status = report.failed_resources? ? 'failed' : 
report.changed_resources? ? 'changed' : 'unchanged'
+      report.save!
+      report.update_node(true)
+      report
     end
-
-    report.status = report.failed_resources? ? 'failed' : 
report.changed_resources? ? 'changed' : 'unchanged'
-    report.save!
-    report.update_node(true)
-    report
   end
 
   def assign_to_node
diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb
index 1c4f61a..9552a4c 100644
--- a/spec/models/report_spec.rb
+++ b/spec/models/report_spec.rb
@@ -13,6 +13,17 @@ describe Report do
         @report_data = YAML.load(@report_yaml).extend(ReportExtensions)
       end
 
+      it "should recover from errors without polluting the database" do
+        Report.count.should == 0
+        yaml = <<HEREDOC
+--- !ruby/object:Puppet::Transaction::Report
+  time: 2010-07-08 12:35:46.027576 -04:00
+  host: localhost.localdomain
+HEREDOC
+        lambda { Report.create_from_yaml(yaml) }.should raise_exception
+        Report.count.should == 0
+      end
+
       it "sets status correctly based on whether the report contains failures" 
do
         report = Report.create_from_yaml(File.read(File.join(Rails.root, 
'spec/fixtures/reports/failure.yml')))
         report.status.should == 'failed'
-- 
1.7.3.1

-- 
You received this message because you are subscribed to the Google Groups 
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to 
[email protected].
For more options, visit this group at 
http://groups.google.com/group/puppet-dev?hl=en.

Reply via email to