Finishing the refactoring of the resource generation interface.

All of the code works, and there are integration tests all around
to prove it.  I think.

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

5 files changed, 74 insertions, 208 deletions

diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index 333e8965a..0f94f5a12 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -160,7 +160,7 @@ class Transaction
 
     # Copy an important relationships from the parent to the newly-generated
     # child resource.
-    def copy_relationships(resource, children)
+    def make_parent_child_relationship(resource, children)
         depthfirst = resource.depthfirst?
         
         children.each do |gen_child|
@@ -169,7 +169,7 @@ class Transaction
             else
                 edge = [resource, gen_child]
             end
-            relationship_graph.add_resource(gen_child) unless relationship_graph.resource(gen_child.ref)
+            relationship_graph.add_vertex(gen_child)
 
             unless relationship_graph.edge?(edge[1], edge[0])
                 relationship_graph.add_edge(*edge)
@@ -228,13 +228,6 @@ class Transaction
                 end
             end
 
-            # Create a child/parent relationship.  We do this after everything else because
-            # we want explicit relationships to be able to override automatic relationships,
-            # including this one.
-            if children
-                copy_relationships(resource, children)
-            end
-            
             # A bit of hackery here -- if skipcheck is true, then we're the
             # top-level resource.  If that's the case, then make sure all of
             # the changes list this resource as a proxy.  This is really only
@@ -353,6 +346,8 @@ class Transaction
         made.each do |res|
             @catalog.add_resource(res) { |r| r.finish }
         end
+        make_parent_child_relationship(resource, made)
+        made
     end
 
     # Collect any dynamically generated resources.  This method is called
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index dfb909f3f..ccef61253 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -331,16 +331,15 @@ module Puppet
         
         # Create any children via recursion or whatever.
         def eval_generate
-            return nil unless self.recurse?
+            return [] unless self.recurse?
 
-            raise(Puppet::DevError, "Cannot generate resources for recursion without a catalog") unless catalog
-
-            recurse.reject do |resource|
-                catalog.resource(:file, resource[:path])
-            end.each do |child|
-                catalog.add_resource child
-                catalog.relationship_graph.add_edge self, child
-            end
+            recurse
+            #recurse.reject do |resource|
+            #    catalog.resource(:file, resource[:path])
+            #end.each do |child|
+            #    catalog.add_resource child
+            #    catalog.relationship_graph.add_edge self, child
+            #end
         end
 
         def flush
@@ -487,7 +486,6 @@ module Puppet
             end
 
             return self.class.create(options)
-            #return catalog.create_implicit_resource(:file, options)
         end
 
         # Files handle paths specially, because they just lengthen their
@@ -517,10 +515,6 @@ module Puppet
             @parameters.include?(:purge) and (self[:purge] == :true or self[:purge] == "true")
         end
 
-        def make_children(metadata)
-            metadata.collect { |meta| newchild(meta.relative_path) }
-        end
-
         # Recursively generate a list of file resources, which will
         # be used to copy remote files, manage local files, and/or make links
         # to map to another directory.
diff --git a/spec/integration/type/file.rb b/spec/integration/type/file.rb
index 5e32332d3..9d1b77788 100755
--- a/spec/integration/type/file.rb
+++ b/spec/integration/type/file.rb
@@ -119,6 +119,44 @@ describe Puppet::Type.type(:file) do
         end
     end
 
+    describe "when generating resources" do
+        before do
+            @source = tmpfile("generating_in_catalog_source")
+
+            @dest = tmpfile("generating_in_catalog_dest")
+
+            Dir.mkdir(@source)
+
+            s1 = File.join(@source, "one")
+            s2 = File.join(@source, "two")
+
+            File.open(s1, "w") { |f| f.puts "uno" }
+            File.open(s2, "w") { |f| f.puts "dos" }
+
+            @file = Puppet::Type::File.create(:name => @dest, :source => @source, :recurse => true)
+
+            @catalog = Puppet::Node::Catalog.new
+            @catalog.add_resource @file
+        end
+
+        it "should add each generated resource to the catalog" do
+            @catalog.apply do |trans|
+                @catalog.resource(:file, File.join(@dest, "one")).should be_instance_of(@file.class)
+                @catalog.resource(:file, File.join(@dest, "two")).should be_instance_of(@file.class)
+            end
+        end
+        
+        it "should have an edge to each resource in the relationship graph" do
+            @catalog.apply do |trans|
+                one = @catalog.resource(:file, File.join(@dest, "one"))
+                @catalog.relationship_graph.should be_edge(@file, one)
+
+                two = @catalog.resource(:file, File.join(@dest, "two"))
+                @catalog.relationship_graph.should be_edge(@file, two)
+            end
+        end
+    end
+
     describe "when copying files" do
         # Ticket #285.
         it "should be able to copy files with pound signs in their names" do
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 0573ea744..196f8cbb8 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -45,6 +45,17 @@ describe Puppet::Transaction do
             @catalog.resource(:generator, "two").should equal(two)
         end
 
+        it "should add an edge in the relationship graph between the generating and generated resource" do
+            one = @generator_class.create :name => "one"
+            two = @generator_class.create :name => "two"
+            @generator.expects(:generate).returns [one]
+            one.expects(:generate).returns [two]
+            @transaction.generate
+
+            @catalog.relationship_graph.should be_edge(@generator, one)
+            @catalog.relationship_graph.should be_edge(one, two)
+        end
+
         it "should finish all non-conflicting resources" do
             one = @generator_class.create :name => "one"
             one.expects(:finish)
@@ -68,6 +79,14 @@ describe Puppet::Transaction do
                 @catalog.resource(:generator, "two").should equal(two)
             end
 
+            it "should add an edge in the relationship graph between the generating and generated resource" do
+                one = @generator_class.create :name => "one"
+                @generator.expects(:eval_generate).returns [one]
+                @transaction.eval_generate(@generator)
+
+                @catalog.relationship_graph.should be_edge(@generator, one)
+            end
+
             it "should not recursively eval_generate resources" do
                 one = @generator_class.create :name => "one"
                 two = @generator_class.create :name => "two"
@@ -89,18 +108,12 @@ describe Puppet::Transaction do
         # Create a bogus type that generates new instances with shorter names
         type = Puppet::Type.newtype(:generator) do
             newparam(:name, :namevar => true)
+
+            # Stub methods.
             def generate
-                ret = []
-                if title.length > 1
-                    ret << self.class.create(:title => title[0..-2])
-                else
-                    return nil
-                end
-                ret
             end
 
             def eval_generate
-                generate
             end
 
             def finished?
@@ -114,134 +127,4 @@ describe Puppet::Transaction do
         
         return type
     end
-    
-    # Test pre-evaluation generation
-    def test_generate
-        mkgenerator() do
-            def generate
-                ret = []
-                if title.length > 1
-                    ret << self.class.create(:title => title[0..-2])
-                else
-                    return nil
-                end
-                ret
-            end
-        end
-        
-        yay = Puppet::Type.newgenerator :title => "yay"
-        rah = Puppet::Type.newgenerator :title => "rah"
-        catalog = mk_catalog(yay, rah)
-        trans = Puppet::Transaction.new(catalog)
-        
-        assert_nothing_raised do
-            trans.generate
-        end
-        
-        %w{ya ra y r}.each do |name|
-            assert(catalog.resource(:generator, name),
-                "Generated %s was not a vertex" % name)
-            assert($finished.include?(name), "%s was not finished" % name)
-        end
-        
-        # Now make sure that cleanup gets rid of those generated types.
-        assert_nothing_raised do
-            trans.cleanup
-        end
-        
-        %w{ya ra y r}.each do |name|
-            assert(! catalog.resource(:generator, name),
-                "Generated vertex %s was not removed from graph" % name)
-        end
-    end
-    
-    # Test mid-evaluation generation.
-    def test_eval_generate
-        $evaluated = []
-        cleanup { $evaluated = nil }
-        type = mkreducer() do
-            def evaluate
-                $evaluated << self.title
-                return []
-            end
-        end
-
-        yay = Puppet::Type.newgenerator :title => "yay"
-        rah = Puppet::Type.newgenerator :title => "rah", :subscribe => yay
-        catalog = mk_catalog(yay, rah)
-        trans = Puppet::Transaction.new(catalog)
-        
-        trans.prepare
-        
-        # Now apply the resources, and make sure they appropriately generate
-        # things.
-        assert_nothing_raised("failed to apply yay") do
-            trans.eval_resource(yay)
-        end
-        ya = catalog.resource(type.name, "ya")
-        assert(ya, "Did not generate ya")
-        assert(trans.relationship_graph.vertex?(ya),
-            "Did not add ya to rel_graph")
-        
-        # Now make sure the appropriate relationships were added
-        assert(trans.relationship_graph.edge?(yay, ya),
-            "parent was not required by child")
-        assert(! trans.relationship_graph.edge?(ya, rah),
-            "generated child ya inherited depencency on rah")
-        
-        # Now make sure it in turn eval_generates appropriately
-        assert_nothing_raised("failed to apply yay") do
-            trans.eval_resource(catalog.resource(type.name, "ya"))
-        end
-
-        %w{y}.each do |name|
-            res = catalog.resource(type.name, "ya")
-            assert(res, "Did not generate %s" % name)
-            assert(trans.relationship_graph.vertex?(res),
-                "Did not add %s to rel_graph" % name)
-            assert($finished.include?("y"), "y was not finished")
-        end
-        
-        assert_nothing_raised("failed to eval_generate with nil response") do
-            trans.eval_resource(catalog.resource(type.name, "y"))
-        end
-        assert(trans.relationship_graph.edge?(yay, ya), "no edge was created for ya => yay")
-        
-        assert_nothing_raised("failed to apply rah") do
-            trans.eval_resource(rah)
-        end
-
-        ra = catalog.resource(type.name, "ra")
-        assert(ra, "Did not generate ra")
-        assert(trans.relationship_graph.vertex?(ra),
-            "Did not add ra to rel_graph" % name)
-        assert($finished.include?("ra"), "y was not finished")
-        
-        # Now make sure this generated resource has the same relationships as
-        # the generating resource
-        assert(! trans.relationship_graph.edge?(yay, ra),
-           "rah passed its dependencies on to its children")
-        assert(! trans.relationship_graph.edge?(ya, ra),
-            "children have a direct relationship")
-        
-        # Now make sure that cleanup gets rid of those generated types.
-        assert_nothing_raised do
-            trans.cleanup
-        end
-        
-        %w{ya ra y r}.each do |name|
-            assert(!trans.relationship_graph.vertex?(catalog.resource(type.name, name)),
-                "Generated vertex %s was not removed from graph" % name)
-        end
-        
-        # Now, start over and make sure that everything gets evaluated.
-        trans = Puppet::Transaction.new(catalog)
-        $evaluated.clear
-        assert_nothing_raised do
-            trans.evaluate
-        end
-        
-        assert_equal(%w{yay ya y rah ra r}, $evaluated,
-            "Not all resources were evaluated or not in the right order")
-    end
 end
diff --git a/spec/unit/type/file.rb b/spec/unit/type/file.rb
index 8c1eef177..bb0fc10f3 100755
--- a/spec/unit/type/file.rb
+++ b/spec/unit/type/file.rb
@@ -519,51 +519,17 @@ describe Puppet::Type.type(:file) do
         it "should not recurse if recursion is disabled" do
             @file.expects(:recurse?).returns false
             @file.expects(:recurse).never
-            @file.eval_generate.should be_nil
+            @file.eval_generate.should == []
         end
 
-        it "should fail if no catalog is set" do
-            @file.catalog = nil
-            lambda { @file.eval_generate }.should raise_error(Puppet::DevError)
-        end
-
-        it "should skip resources that are already in the catalog" do
-            foo = stub 'foo', :[] => "/foo"
-            bar = stub 'bar', :[] => "/bar"
-            bar2 = stub 'bar2', :[] => "/bar"
-
-            @catalog.expects(:resource).with(:file, "/foo").returns nil
-            @catalog.expects(:resource).with(:file, "/bar").returns bar2
-
-            @file.expects(:recurse).returns [foo, bar]
-
-            @file.eval_generate.should == [foo]
-        end
-
-        it "should add each resource to the catalog" do
+        it "should return each resource found through recursion" do
             foo = stub 'foo', :[] => "/foo"
             bar = stub 'bar', :[] => "/bar"
             bar2 = stub 'bar2', :[] => "/bar"
 
-            @catalog.expects(:add_resource).with(foo)
-            @catalog.expects(:add_resource).with(bar)
-
             @file.expects(:recurse).returns [foo, bar]
 
-            @file.eval_generate
-        end
-
-        it "should add a relationshp edge for each returned resource" do
-            foo = stub 'foo', :[] => "/foo"
-
-            @file.expects(:recurse).returns [foo]
-
-            graph = mock 'graph'
-            @catalog.stubs(:relationship_graph).returns graph
-
-            graph.expects(:add_edge).with(@file, foo)
-
-            @file.eval_generate
+            @file.eval_generate.should == [foo, bar]
         end
     end
 
@@ -626,11 +592,6 @@ describe Puppet::Type.type(:file) do
                 end
             end
 
-            it "should fail if it has no catalog" do
-                file = @file.class.create(:path => "/foo/bar", :owner => "root", :group => "wheel")
-                lambda { file.newchild("foo/bar").should raise_error(ArgumentError)
-            end
-
             it "should copy all of the parent resource's 'should' values that were set at initialization" do
                 file = @file.class.create(:path => "/foo/bar", :owner => "root", :group => "wheel")
                 @catalog.add_resource(file)
@@ -653,11 +614,6 @@ describe Puppet::Type.type(:file) do
                 @file.class.expects(:create).with { |params| params[:group].nil? }
                 @file.newchild("my/path")
             end
-
-            it "should return nil if the specified child resource already exists in the catalog" do
-                @catalog.expects(:resource).with(:file, File.join(@file[:path], "foo/bar")).returns mock("resource")
-                @file.newchild("foo/bar").should be_nil
-            end
         end
     end
 end