Fixing #2658 - adding backward compatibility for 0.24

The way stages were implemented caused backward compatibility
to be completely broken for 0.24.x.

This commit fixes that, mostly by assuming Stage[main] will be the
top node in the graph rather than Class[main].

Other stages are not supported in 0.24.x, and explicitly throw a warning
(although not an error).

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

lib/puppet/parser/compiler.rb

@@ -54,10 +54,10 @@ class Puppet::Parser::Compiler
         set_container_resource(scope, resource)
     end
 
+    # Add our container edge.  If we're a class, then we get treated specially - we can
+    # control the stage that the class is applied in.  Otherwise, we just
+    # get added to our parent container.
     def set_container_resource(scope, resource)
-        # Add our container edge.  If we're a class, then we get treated specially - we can
-        # control the stage that the class is applied in.  Otherwise, we just
-        # get added to our parent container.
         return if resource.type.to_s.downcase == "stage"
 
         if resource.type.to_s.downcase != "class"
@@ -305,6 +305,8 @@ class Puppet::Parser::Compiler
         @resources << @main_resource
         @catalog.add_resource(@main_resource)
 
+        set_container_resource(@topscope, @main_resource)
+
         @main_resource.evaluate
     end
 

lib/puppet/resource.rb

@@ -267,7 +267,7 @@ class Puppet::Resource
 
     # Translate our object to a backward-compatible transportable object.
     def to_trans
-        if builtin_type?
+        if builtin_type? and type.downcase.to_s != "stage"
             result = to_transobject
         else
             result = to_transbucket

lib/puppet/resource/catalog.rb

@@ -33,9 +33,6 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     # How long this catalog took to retrieve.  Used for reporting stats.
     attr_accessor :retrieval_duration
 
-    # How we should extract the catalog for sending to the client.
-    attr_reader :extraction_format
-
     # Whether this is a host catalog, which behaves very differently.
     # In particular, reports are sent, graphs are made, and state is
     # stored in the state database.  If this is set incorrectly, then you often
@@ -201,33 +198,22 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
         end
     end
 
-    # Make sure we support the requested extraction format.
-    def extraction_format=(value)
-        unless respond_to?("extract_to_%s" % value)
-            raise ArgumentError, "Invalid extraction format %s" % value
-        end
-        @extraction_format = value
-    end
-
-    # Turn our catalog graph into whatever the client is expecting.
-    def extract
-        send("extract_to_%s" % extraction_format)
-    end
-
-    # Create the traditional TransBuckets and TransObjects from our catalog
-    # graph.  LAK:NOTE(20081211): This is a  pre-0.25 backward compatibility method.
+    # Turn our catalog graph into an old-style tree of TransObjects and TransBuckets.
+    # LAK:NOTE(20081211): This is a  pre-0.25 backward compatibility method.
     # It can be removed as soon as xmlrpc is killed.
-    def extract_to_transportable
+    def extract
         top = nil
         current = nil
         buckets = {}
 
-        unless main = vertices.find { |res| res.type == "Class" and res.title == :main }
-            raise Puppet::DevError, "Could not find 'main' class; cannot generate catalog"
+        unless main = resource(:stage, "main")
+            raise Puppet::DevError, "Could not find 'main' stage; cannot generate catalog"
+        end
+
+        if stages = vertices.find_all { |v| v.type == "Stage" and v.title != "main" } and ! stages.empty?
+            Puppet.warning "Stages are not supported by 0.24.x client; stage(s) #{stages.collect { |s| s.to_s }.join(', ') } will be ignored"
         end
 
-        # Create a proc for examining edges, which we'll use to build our tree
-        # of TransBuckets and TransObjects.
         bucket = nil
         walk(main, :out) do |source, target|
             # The sources are always non-builtins.
@@ -285,7 +271,6 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     def initialize(name = nil)
         super()
         @name = name if name
-        @extraction_format ||= :transportable
         @classes = []
         @resource_table = {}
         @transient_resources = []

lib/puppet/type.rb

@@ -1407,24 +1407,24 @@ class Type
             a dependency on or from the named milestone.  For instance, saying that
             this is in the 'bootstrap' stage creates a dependency on the 'bootstrap'
             milestone.
-        
+
             By default, all classes get directly added to the
             'main' stage.  You can create new stages as resources:
-            
+
                 stage { [pre, post]: }
-            
+
             To order stages, use standard relationships:
-            
+
                 stage { pre: before => Stage[main] }
-            
+
             Or use the new relationship syntax:
-            
+
                 Stage[pre] -> Stage[main] -> Stage[post]
-            
+
             Then use the new class parameters to specify a stage:
-            
+
                 class { foo: stage => pre }
-            
+
             Stages can only be set on classes, not individual resources.  This will
             fail::
 

lib/puppet/type/stage.rb

@@ -2,11 +2,11 @@ Puppet::Type.newtype(:stage) do
     desc "A resource type for specifying run stages.  The actual stage should
     be specified on resources::
         class { foo: stage => pre }
-        
+
     And you must manually control stage order::
-    
+
         stage { pre: before => Stage[main] }
-        
+
     You automatically get a 'main' stage created, and by default all resources
     get inserted into that stage.
 

spec/unit/parser/compiler.rb

@@ -200,6 +200,14 @@ describe Puppet::Parser::Compiler do
             @known_resource_types.find_hostclass([""], "").should be_instance_of(Puppet::Resource::Type)
         end
 
+        it "should add an edge between the main stage and main class" do
+            @compiler.compile
+            (stage = @compiler.catalog.resource(:stage, "main")).should be_instance_of(Puppet::Parser::Resource)
+            (klass = @compiler.catalog.resource(:class, "")).should be_instance_of(Puppet::Parser::Resource)
+
+            @compiler.catalog.edge?(stage, klass).should be_true
+        end
+
         it "should evaluate any node classes" do
             @node.stubs(:classes).returns(%w{one two three four})
             @compiler.expects(:evaluate_classes).with(%w{one two three four}, @compiler.topscope)

spec/unit/resource.rb

@@ -533,6 +533,14 @@ describe Puppet::Resource do
                 bucket.name.should == @resource.title
             end
 
+            it "should return a simple TransBucket if it is a stage" do
+                @resource = Puppet::Resource.new("stage", "bar")
+                bucket = @resource.to_trans
+                bucket.should be_instance_of(Puppet::TransBucket)
+                bucket.type.should == @resource.type
+                bucket.name.should == @resource.title
+            end
+
             it "should copy over the resource's file" do
                 @resource.file = "/foo/bar"
                 @resource.to_trans.file.should == "/foo/bar"

spec/unit/resource/catalog.rb

@@ -88,15 +88,6 @@ describe Puppet::Resource::Catalog, "when compiling" do
         end
     end
 
-    describe "when extracting" do
-        it "should return extraction result as the method result" do
-            config = Puppet::Resource::Catalog.new("mynode")
-            config.expects(:extraction_format).returns(:whatever)
-            config.expects(:extract_to_whatever).returns(:result)
-            config.extract.should == :result
-        end
-    end
-
     describe "when extracting transobjects" do
 
         def mkscope
@@ -112,23 +103,39 @@ describe Puppet::Resource::Catalog, "when compiling" do
             Puppet::Parser::Resource.new(type, name, :source => @source, :scope => @scope)
         end
 
-        it "should always create a TransBucket for the 'main' class" do
+        it "should fail if no 'main' stage can be found" do
+            lambda { Puppet::Resource::Catalog.new("mynode").extract }.should raise_error(Puppet::DevError)
+        end
+
+        it "should warn if any non-main stages are present" do
             config = Puppet::Resource::Catalog.new("mynode")
 
             @scope = mkscope
             @source = mock 'source'
 
-            main = mkresource("class", :main)
-            config.add_vertex(main)
+            main = mkresource("stage", "main")
+            config.add_resource(main)
 
-            bucket = stub 'bucket', :file= => nil, :line= => nil, :classes= => nil
-            bucket.expects(:type=).with("Class")
-            bucket.expects(:name=).with(:main)
-            main.stubs(:builtin?).returns(false)
+            other = mkresource("stage", "other")
+            config.add_resource(other)
 
-            Puppet::TransBucket.expects(:new).returns bucket
+            Puppet.expects(:warning)
 
-            config.extract_to_transportable.should equal(bucket)
+            config.extract
+        end
+
+        it "should always create a TransBucket for the 'main' stage" do
+            config = Puppet::Resource::Catalog.new("mynode")
+
+            @scope = mkscope
+            @source = mock 'source'
+
+            main = mkresource("stage", "main")
+            config.add_resource(main)
+
+            result = config.extract
+            result.type.should == "Stage"
+            result.name.should == "main"
         end
 
         # Now try it with a more complicated graph -- a three tier graph, each tier
@@ -140,7 +147,9 @@ describe Puppet::Resource::Catalog, "when compiling" do
             @source = mock 'source'
 
             # Create our scopes.
-            top = mkresource "class", :main
+            top = mkresource "stage", "main"
+
+            config.add_resource top
             topbucket = []
             topbucket.expects(:classes=).with([])
             top.expects(:to_trans).returns(topbucket)
@@ -162,7 +171,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
             botres.expects(:to_trans).returns(:botres)
             config.add_edge bottom, botres
 
-            toparray = config.extract_to_transportable
+            toparray = config.extract
 
             # This is annoying; it should look like:
             #   [[[:botres], :midres], :topres]

test/language/scope.rb

@@ -201,13 +201,9 @@ Host <<||>>"
         node = mknode
         node.merge "hostname" => node.name
         2.times { |i|
-            config = Puppet::Parser::Compiler.new(node).compile
-
-            flat = config.extract.flatten
-
-            %w{puppet myhost}.each do |name|
-                assert(flat.find{|o| o.name == name }, "Did not find #{name}")
-            end
+            catalog = Puppet::Parser::Compiler.new(node).compile
+            assert_instance_of(Puppet::Parser::Resource, catalog.resource(:host, "puppet"))
+            assert_instance_of(Puppet::Parser::Resource, catalog.resource(:host, "myhost"))
         }
     ensure
         Puppet[:storeconfigs] = false

test/language/snippets.rb

@@ -204,11 +204,7 @@ class TestSnippets < Test::Unit::TestCase
         file = @catalog.resource(:file, path)
         assert(file, "did not create file %s" % path)
 
-        assert_nothing_raised {
-            assert_equal(
-                "//Testing/Mytype[componentname]/File[/tmp/classtest]",
-                file.path)
-        }
+        assert_equal( "/Stage[main]/Testing/Mytype[componentname]/File[/tmp/classtest]", file.path)
     end
 
     def snippet_argumentdefaults

test/lib/puppettest/parsertesting.rb

@@ -297,21 +297,11 @@ module PuppetTest::ParserTesting
 
     # This assumes no nodes
     def assert_creates(manifest, *files)
-        interp = nil
         oldmanifest = Puppet[:manifest]
         Puppet[:manifest] = manifest
 
-        trans = nil
-        assert_nothing_raised {
-            trans = Puppet::Parser::Compiler.new(mknode).compile
-        }
-
-        config = nil
-        assert_nothing_raised {
-            config = trans.extract.to_catalog
-        }
-
-        config.apply
+        catalog = Puppet::Parser::Compiler.new(mknode).compile.to_ral
+        catalog.apply
 
         files.each do |file|
             assert(FileTest.exists?(file), "Did not create %s" % file)

test/ral/type/file/target.rb

@@ -69,7 +69,6 @@ class TestFileTarget < Test::Unit::TestCase
         assert_equal(file, File.readlink(linkpath))
 
         # Use classes for comparison, because the resource inspection is so large
-        assert_equal(NilClass, catalog.resource(:file, sublink).class, "dynamically generated resources were not removed")
         assert_events([], link, "Link is not in sync")
     end