Fixing #2112 - Transactions handle conflicting generated resources

This commit rips out all of the 'implicit resource' crap,
replacing it with a simple system that just skips
resources that the catalog says are in conflict.

Removes a bunch of code, and fixes the bug to boot.

Signed-off-by: Luke Kanies <luke@madstop.com>

10 files changed, 54 insertions, 91 deletions

diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
index add32b7cf..50663cb67 100644
--- a/lib/puppet/resource.rb
+++ b/lib/puppet/resource.rb
@@ -7,7 +7,7 @@ require 'puppet/resource/reference'
 class Puppet::Resource
     include Puppet::Util::Tagging
     include Enumerable
-    attr_accessor :type, :title, :file, :line, :catalog, :implicit
+    attr_accessor :type, :title, :file, :line, :catalog
 
     # Proxy these methods to the parameters hash.  It's likely they'll
     # be overridden at some point, but this works for now.
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 266f04903..88aa9517d 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -65,7 +65,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
             unless resource.respond_to?(:ref)
                 raise ArgumentError, "Can only add objects that respond to :ref, not instances of %s" % resource.class
             end
-        end.find_all { |resource| fail_or_skip_unless_unique(resource) }.each do |resource|
+        end.each { |resource| fail_unless_unique(resource) }.each do |resource|
             ref = resource.ref
 
             @transient_resources << resource if applying?
@@ -430,20 +430,9 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     end
 
     # Verify that the given resource isn't defined elsewhere.
-    def fail_or_skip_unless_unique(resource)
+    def fail_unless_unique(resource)
         # Short-curcuit the common case, 
-        return resource unless existing_resource = @resource_table[resource.ref]
-
-        if resource.implicit?
-            resource.debug "Generated resource conflicts with explicit resource; ignoring generated resource"
-            return nil
-        elsif old = resource(resource.ref) and old.implicit?
-            # The existing resource is implicit; remove it and replace it with
-            # the new one.
-            old.debug "Replacing with new resource"
-            remove_resource(old)
-            return resource
-        end
+        return unless existing_resource = @resource_table[resource.ref]
 
         # If we've gotten this far, it's a real conflict
 
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index bb5adc383..37f51b2e5 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -343,9 +343,15 @@ class Transaction
         made = [made] unless made.is_a?(Array)
         made.uniq!
         made.each do |res|
-            @catalog.add_resource(res) { |r| r.finish }
+            begin
+                @catalog.add_resource(res) do |r|
+                    r.finish
+                    make_parent_child_relationship(resource, [r])
+                end
+            rescue Puppet::Resource::Catalog::DuplicateResourceError
+                next
+            end
         end
-        make_parent_child_relationship(resource, made)
         made
     end
 
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 64cfc2540..311878841 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -617,10 +617,6 @@ class Type
 
     public
 
-    ###############################
-    # Code related to the closure-like behaviour of the resource classes.
-    attr_accessor :implicit
-
     # Is this type's name isomorphic with the object?  That is, if the
     # name conflicts, does it necessarily mean that the objects conflict?
     # Defaults to true.
@@ -632,14 +628,6 @@ class Type
         end
     end
 
-    def implicit?
-        if defined? @implicit and @implicit
-            return true
-        else
-            return false
-        end
-    end
-
     def isomorphic?
         self.class.isomorphic?
     end
@@ -1056,7 +1044,7 @@ class Type
 
         # Now create our resource.
         resource = Puppet::Resource.new(self.name, title)
-        [:catalog, :implicit].each do |attribute|
+        [:catalog].each do |attribute|
             if value = hash[attribute]
                 hash.delete(attribute)
                 resource.send(attribute.to_s + "=", value)
@@ -1914,7 +1902,7 @@ class Type
             self.title = resource.ref
         end
 
-        [:file, :line, :catalog, :implicit].each do |getter|
+        [:file, :line, :catalog].each do |getter|
             setter = getter.to_s + "="
             if val = resource.send(getter)
                 self.send(setter, val)
@@ -2012,13 +2000,7 @@ class Type
         return nil unless catalog
 
         unless defined?(@parent)
-            # This is kinda weird.
-            if implicit?
-                parents = catalog.relationship_graph.adjacent(self, :direction => :in)
-            else
-                parents = catalog.adjacent(self, :direction => :in)
-            end
-            if parents
+            if parents = catalog.adjacent(self, :direction => :in)
                 # We should never have more than one parent, so let's just ignore
                 # it if we happen to.
                 @parent = parents.shift
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 0cd9ebece..344fcb713 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -516,7 +516,7 @@ module Puppet
             # or so.  Unfortunately, we don't have a straightforward way to manage
             # the different lifetimes of this data, so we kludge it like this.
             # The right-side hash wins in the merge.
-            options = @original_parameters.merge(:path => full_path, :implicit => true).reject { |param, value| value.nil? }
+            options = @original_parameters.merge(:path => full_path).reject { |param, value| value.nil? }
 
             # These should never be passed to our children.
             [:parent, :ensure, :recurse, :recurselimit, :target, :alias, :source].each do |param|
diff --git a/spec/unit/resource.rb b/spec/unit/resource.rb
index 0fcf51b8b..141986f90 100755
--- a/spec/unit/resource.rb
+++ b/spec/unit/resource.rb
@@ -4,7 +4,7 @@ require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/resource'
 
 describe Puppet::Resource do
-    [:catalog, :file, :line, :implicit].each do |attr|
+    [:catalog, :file, :line].each do |attr|
         it "should have an #{attr} attribute" do
             resource = Puppet::Resource.new("file", "/my/file")
             resource.should respond_to(attr)
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index f72162b39..6a5922e2e 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -350,50 +350,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
             proc { @catalog.add_resource(@dupe) }.should raise_error(Puppet::Resource::Catalog::DuplicateResourceError)
         end
 
-        it "should ignore implicit resources that conflict with existing resources" do
-            @catalog.add_resource(@one)
-
-            @dupe.implicit = true
-
-            yielded = []
-            @catalog.add_resource(@dupe) { |r| yielded << r }
-            yielded.should be_empty
-        end
-
-        it "should not set the catalog for ignored implicit resources" do
-            @catalog.add_resource(@one)
-
-            @dupe.implicit = true
-
-            @catalog.add_resource(@dupe)
-            @dupe.catalog.should be_nil
-        end
-
-        it "should log when implicit resources are ignored" do
-            @catalog.add_resource(@one)
-
-            @dupe.implicit = true
-
-            @dupe.expects(:debug)
-            @catalog.add_resource(@dupe)
-        end
-
-        it "should replace implicit resources if a conflicting explicit resource is added" do
-            @catalog.add_resource(@one)
-            @one.implicit = true
-
-            proc { @catalog.add_resource(@dupe) }.should_not raise_error
-            @catalog.resource(:notify, "one").should equal(@dupe)
-        end
-
-        it "should log when implicit resources are removed from the catalog" do
-            @catalog.add_resource(@one)
-            @one.implicit = true
-
-            @one.expects(:debug)
-            @catalog.add_resource(@dupe)
-        end
-
         it "should not store objects that do not respond to :ref" do
             proc { @catalog.add_resource("thing") }.should raise_error(ArgumentError)
         end
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 60705c7fb..86ee02ce5 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -17,6 +17,40 @@ describe Puppet::Transaction do
 
         @transaction.prefetch
     end
+
+    describe "when generating resources" do
+        it "should finish all resources" do
+            generator = stub 'generator', :depthfirst? => true
+            resource = stub 'resource'
+
+            @catalog = Puppet::Resource::Catalog.new
+            @transaction = Puppet::Transaction.new(@catalog)
+
+            generator.expects(:generate).returns [resource]
+
+            @catalog.expects(:add_resource).yields(resource)
+
+            resource.expects(:finish)
+
+            @transaction.generate_additional_resources(generator, :generate)
+        end
+
+        it "should skip generated resources that conflict with existing resources" do
+            generator = mock 'generator'
+            resource = stub 'resource'
+
+            @catalog = Puppet::Resource::Catalog.new
+            @transaction = Puppet::Transaction.new(@catalog)
+
+            generator.expects(:generate).returns [resource]
+
+            @catalog.expects(:add_resource).raises(Puppet::Resource::Catalog::DuplicateResourceError.new("foo"))
+
+            resource.expects(:finish).never
+
+            @transaction.generate_additional_resources(generator, :generate)
+        end
+    end
 end
 
 describe Puppet::Transaction, " when determining tags" do
diff --git a/spec/unit/type.rb b/spec/unit/type.rb
index 3523d4e8b..304eb4017 100755
--- a/spec/unit/type.rb
+++ b/spec/unit/type.rb
@@ -61,7 +61,7 @@ describe Puppet::Type do
                 Puppet::Type.type(:mount).new(resource).title.should == "User[foo]"
             end
 
-            [:line, :file, :catalog, :implicit].each do |param|
+            [:line, :file, :catalog].each do |param|
                 it "should copy '#{param}' from the resource if present" do
                     resource = Puppet::Resource.new(:mount, "/foo")
                     resource.send(param.to_s + "=", "foo")
@@ -112,7 +112,7 @@ describe Puppet::Type do
                 @type.stubs(:namevar).returns :myname
             end
 
-            [:catalog, :implicit].each do |param|
+            [:catalog].each do |param|
                 it "should extract '#{param}' from the hash if present" do
                     Puppet::Type.type(:mount).new(:name => "/yay", param => "foo").send(param).should == "foo"
                 end
@@ -220,7 +220,7 @@ describe Puppet::Type do
             lambda { @type.hash2resource(:myname => "foo", :name => 'bar') }.should raise_error(Puppet::Error)
         end
 
-        [:catalog, :implicit].each do |attr|
+        [:catalog].each do |attr|
             it "should use any provided #{attr}" do
                 @type.hash2resource(:name => "foo", attr => "eh").send(attr).should == "eh"
             end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 094596966..1c6976440 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -609,10 +609,6 @@ describe Puppet::Type.type(:file) do
         end
 
         describe "and making a new child resource" do
-            it "should create an implicit resource using the provided relative path joined with the file's path" do
-                @file.newchild("my/path").should be_implicit
-            end
-
             it "should not copy the parent resource's parent" do
                 Puppet::Type.type(:file).expects(:new).with { |options| ! options.include?(:parent) }
                 @file.newchild("my/path")