Refactoring how the catalog creation handles errors.

Previously, if there were an error creating a resource,
the error would propagate leaving any previously
created resources still in memory.

Now, resources are removed by default when an error
happens during instantiation, and the error propagates
to the caller so that they can log or whatever.

This also allows the Settings class to correctly and
separately handle the cases where we can't create the
catalog (which should never happen in normal usage but
was happening because of this error -- later catalogs
couldn't be created because earlier catalogs left
resources lying around) from those where we can't
apply the catalog.

4 files changed, 72 insertions, 50 deletions

diff --git a/lib/puppet/transportable.rb b/lib/puppet/transportable.rb
index f686fbb78..3ad084b3b 100644
--- a/lib/puppet/transportable.rb
+++ b/lib/puppet/transportable.rb
@@ -83,14 +83,11 @@ module Puppet
         end
 
         def to_type
-            retobj = nil
             if typeklass = Puppet::Type.type(self.type)
                 return typeklass.create(self)
             else
                 return to_component
             end
-
-            return retobj
         end
     end
 
@@ -179,28 +176,39 @@ module Puppet
         end
 
         # Create a resource graph from our structure.
-        def to_catalog
-            catalog = Puppet::Node::Catalog.new(Facter.value("hostname")) do |config|
-                delver = proc do |obj|
-                    obj.catalog = config
-                    unless container = config.resource(obj.to_ref)
-                        container = obj.to_type
-                        config.add_resource container
+        def to_catalog(clear_on_failure = true)
+            catalog = Puppet::Node::Catalog.new(Facter.value("hostname"))
+
+            # This should really use the 'delve' method, but this
+            # whole class is going away relatively soon, hopefully,
+            # so it's not worth it.
+            delver = proc do |obj|
+                obj.catalog = catalog
+                unless container = catalog.resource(obj.to_ref)
+                    container = obj.to_type
+                    catalog.add_resource container
+                end
+                obj.each do |child|
+                    child.catalog = catalog
+                    unless resource = catalog.resource(child.to_ref)
+                        resource = child.to_type
+                        catalog.add_resource resource
                     end
-                    obj.each do |child|
-                        child.catalog = config
-                        unless resource = config.resource(child.to_ref)
-                            next unless resource = child.to_type
-                            config.add_resource resource
-                        end
-                        config.add_edge(container, resource)
-                        if child.is_a?(self.class)
-                            delver.call(child)
-                        end
+
+                    catalog.add_edge(container, resource)
+                    if child.is_a?(self.class)
+                        delver.call(child)
                     end
                 end
+            end
                 
+            begin
                 delver.call(self)
+                catalog.finalize
+            rescue => detail
+                # This is important until we lose the global resource references.
+                catalog.clear if (clear_on_failure)
+                raise
             end
             
             return catalog
diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index c6797a767..d27406d6d 100644
--- a/lib/puppet/util/settings.rb
+++ b/lib/puppet/util/settings.rb
@@ -44,19 +44,6 @@ class Puppet::Util::Settings
         return value
     end
 
-    # A simplified equality operator.
-    # LAK: For some reason, this causes mocha to not be able to mock
-    # the 'value' method, and it's not used anywhere.
-#    def ==(other)
-#        self.each { |myname, myobj|
-#            unless other[myname] == value(myname)
-#                return false
-#            end
-#        }
-#
-#        return true
-#    end
-
     # Generate the list of valid arguments, in a format that GetoptLong can
     # understand, and add them to the passed option list.
     def addargs(options)
@@ -674,6 +661,16 @@ Generated on #{Time.now}.
 
             begin
                 catalog = bucket.to_catalog
+            rescue => detail
+                puts detail.backtrace if Puppet[:trace]
+                Puppet.err "Could not create resources for managing Puppet's files and directories: %s" % detail
+
+                # We need some way to get rid of any resources created during the catalog creation
+                # but not cleaned up.
+                return
+            end
+
+            begin
                 catalog.host_config = false
                 catalog.apply do |transaction|
                     if failures = transaction.any_failed?
@@ -681,8 +678,7 @@ Generated on #{Time.now}.
                     end
                 end
             ensure
-                # The catalog won't exist if there was an error creating it.
-                catalog.clear if catalog
+                catalog.clear
             end
 
             sections.each { |s| @used << s }
diff --git a/spec/unit/other/transbucket.rb b/spec/unit/other/transbucket.rb
index 3904e39fe..4494f2abb 100755
--- a/spec/unit/other/transbucket.rb
+++ b/spec/unit/other/transbucket.rb
@@ -87,42 +87,54 @@ describe Puppet::TransBucket, " when generating a catalog" do
         @top.push(@topobj)
         @top.push(@middle)
 
-        @config = @top.to_catalog
-
         @users = %w{top middle bottom}
         @fakes = %w{Fake[bottom] Fake[middle] Fake[top]}
     end
 
+    after do
+        Puppet::Type.allclear
+    end
+
     it "should convert all transportable objects to RAL resources" do
+        @catalog = @top.to_catalog
         @users.each do |name|
-            @config.vertices.find { |r| r.class.name == :user and r.title == name }.should be_instance_of(Puppet::Type.type(:user))
+            @catalog.vertices.find { |r| r.class.name == :user and r.title == name }.should be_instance_of(Puppet::Type.type(:user))
         end
     end
 
+    it "should fail if any transportable resources fail to convert to RAL resources" do
+        @bottomobj.expects(:to_type).raises ArgumentError
+        lambda { @bottom.to_catalog }.should raise_error(ArgumentError)
+    end
+
     it "should convert all transportable buckets to RAL components" do
+        @catalog = @top.to_catalog
         @fakes.each do |name|
-            @config.vertices.find { |r| r.class.name == :component and r.title == name }.should be_instance_of(Puppet::Type.type(:component))
+            @catalog.vertices.find { |r| r.class.name == :component and r.title == name }.should be_instance_of(Puppet::Type.type(:component))
         end
     end
 
     it "should add all resources to the graph's resource table" do
-        @config.resource("fake[top]").should equal(@top)
+        @catalog = @top.to_catalog
+        @catalog.resource("fake[top]").should equal(@top)
     end
 
     it "should finalize all resources" do
-        @config.vertices.each do |vertex| vertex.should be_finalized end
+        @catalog = @top.to_catalog
+        @catalog.vertices.each do |vertex| vertex.should be_finalized end
     end
 
     it "should only call to_type on each resource once" do
-        @topobj.expects(:to_type)
-        @bottomobj.expects(:to_type)
-        Puppet::Type.allclear
+        # We just raise exceptions here because we're not interested in
+        # what happens with the result, only that the method only
+        # gets called once.
+        resource = @topobj.to_type
+        @topobj.expects(:to_type).once.returns resource
         @top.to_catalog
     end
 
     it "should set each TransObject's catalog before converting to a RAL resource" do
         @middleobj.expects(:catalog=).with { |c| c.is_a?(Puppet::Node::Catalog) }
-        Puppet::Type.allclear
         @top.to_catalog
     end
 
@@ -130,13 +142,8 @@ describe Puppet::TransBucket, " when generating a catalog" do
         # each bucket is seen twice in the loop, so we have to handle the case where the config
         # is set twice
         @bottom.expects(:catalog=).with { |c| c.is_a?(Puppet::Node::Catalog) }.at_least_once
-        Puppet::Type.allclear
         @top.to_catalog
     end
-
-    after do
-        Puppet::Type.allclear
-    end
 end
 
 describe Puppet::TransBucket, " when serializing" do
diff --git a/spec/unit/util/settings.rb b/spec/unit/util/settings.rb
index fbd638663..a6b358462 100755
--- a/spec/unit/util/settings.rb
+++ b/spec/unit/util/settings.rb
@@ -605,6 +605,17 @@ describe Puppet::Util::Settings, " when being used to manage the host machine" d
         lambda { trans.to_catalog }.should_not raise_error
     end
 
+    it "should do nothing if a catalog cannot be created" do
+        bucket = mock 'bucket'
+        catalog = mock 'catalog'
+
+        @settings.expects(:to_transportable).returns bucket
+        bucket.expects(:to_catalog).raises RuntimeError
+        catalog.expects(:apply).never
+
+        @settings.use(:mysection)
+    end
+
     it "should clear the catalog after applying" do
         bucket = mock 'bucket'
         catalog = mock 'catalog'