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.

2 files changed, 39 insertions, 35 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 }