Fix #1933 - Inconsistent resource evaluation order in subsequent evaluation runs

While evaluating the AST, catalog vertices are not always ordered
the same way on different run, leading to some tags (which should
have been applied in evaluation order) to not be associated with
some underlying resources.

This changeset change all accesses to resources inside the compiler
to always use an ordered (in evaluation order) list of added
resources.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

2 files changed, 36 insertions, 10 deletions

diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index 9f5548d8b..7dcd50270 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -10,7 +10,7 @@ require 'puppet/util/errors'
 class Puppet::Parser::Compiler
     include Puppet::Util
     include Puppet::Util::Errors
-    attr_reader :parser, :node, :facts, :collections, :catalog, :node_scope
+    attr_reader :parser, :node, :facts, :collections, :catalog, :node_scope, :resources
 
     # Add a collection to the global list.
     def add_collection(coll)
@@ -31,6 +31,8 @@ class Puppet::Parser::Compiler
 
     # Store a resource in our resource table.
     def add_resource(scope, resource)
+        @resources << resource
+
         # Note that this will fail if the resource is not unique.
         @catalog.add_resource(resource)
 
@@ -204,11 +206,6 @@ class Puppet::Parser::Compiler
         @resource_overrides[resource.ref]
     end
 
-    # Return a list of all resources.
-    def resources
-        @catalog.vertices
-    end
-
     # The top scope is usually the top-level scope, but if we're using AST nodes,
     # then it is instead the node's scope.
     def topscope
@@ -310,6 +307,7 @@ class Puppet::Parser::Compiler
         @main_resource = Puppet::Parser::Resource.new(:type => "class", :title => :main, :scope => @topscope, :source => @main)
         @topscope.resource = @main_resource
 
+        @resources << @main_resource
         @catalog.add_resource(@main_resource)
 
         @main_resource.evaluate
@@ -366,7 +364,7 @@ class Puppet::Parser::Compiler
     # Make sure all of our resources and such have done any last work
     # necessary.
     def finish
-        @catalog.vertices.each do |resource|
+        resources.each do |resource|
             # Add in any resource overrides.
             if overrides = resource_overrides(resource)
                 overrides.each do |over|
@@ -414,6 +412,9 @@ class Puppet::Parser::Compiler
         # For maintaining the relationship between scopes and their resources.
         @catalog = Puppet::Resource::Catalog.new(@node.name)
         @catalog.version = @parser.version
+
+        # local resource array to maintain resource ordering
+        @resources = []
     end
 
     # Set the node's parameters into the top-scope as variables.
@@ -436,7 +437,7 @@ class Puppet::Parser::Compiler
 
         # We used to have hooks here for forking and saving, but I don't
         # think it's worth retaining at this point.
-        store_to_active_record(@node, @catalog.vertices)
+        store_to_active_record(@node, resources)
     end
 
     # Do the actual storage.
@@ -459,7 +460,7 @@ class Puppet::Parser::Compiler
     # Return an array of all of the unevaluated resources.  These will be definitions,
     # which need to get evaluated into native resources.
     def unevaluated_resources
-        ary = @catalog.vertices.reject { |resource| resource.builtin? or resource.evaluated?  }
+        ary = resources.reject { |resource| resource.builtin? or resource.evaluated?  }
 
         if ary.empty?
             return nil
diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/compiler.rb
index f36b6fd4f..855a8c723 100755
--- a/spec/unit/parser/compiler.rb
+++ b/spec/unit/parser/compiler.rb
@@ -239,6 +239,31 @@ describe Puppet::Parser::Compiler do
             @compiler.send(:finish)
         end
 
+        it "should call finish() in add_resource order" do
+            resources = sequence('resources')
+
+            resource1 = Puppet::Parser::Resource.new :scope => @scope, :type => "file", :title => "finish1"
+            resource1.expects(:finish).in_sequence(resources)
+
+            @compiler.add_resource(@scope, resource1)
+
+            resource2 = Puppet::Parser::Resource.new :scope => @scope, :type => "file", :title => "finish2"
+            resource2.expects(:finish).in_sequence(resources)
+
+            @compiler.add_resource(@scope, resource2)
+
+            @compiler.send(:finish)
+        end
+
+        it "should return added resources in add order" do
+            resource1 = stub "1", :ref => "File[yay]"
+            @compiler.add_resource(@scope, resource1)
+            resource2 = stub "2", :ref => "File[youpi]"
+            @compiler.add_resource(@scope, resource2)
+
+            @compiler.resources.should == [resource1, resource2]
+        end
+
         it "should add resources that do not conflict with existing resources" do
             resource = stub "noconflict", :ref => "File[yay]"
             @compiler.add_resource(@scope, resource)
@@ -484,7 +509,7 @@ describe Puppet::Parser::Compiler do
             Puppet.features.expects(:rails?).returns(true)
             Puppet::Rails.expects(:connect)
 
-            @compiler.catalog.expects(:vertices).returns(:resources)
+            @compiler.expects(:resources).returns(:resources)
 
             @compiler.expects(:store_to_active_record).with(@node, :resources)
             @compiler.send(:store)