Yep.
It's plenty dangerous. In particular, it's probably pretty easy to get cycles
in your graph now. E.g., say you do this:
Stage[pre] -> Stage[main] # yay new syntax
class apt {
file { "/some/apt/config": ensure => present }
exec { "/usr/bin/apt-get update": require => File[/some/apt/config], stage =>
pre }
}
Boom. You've got a cycle. The config file is still in 'main', the exec is in
'pre', main depends on pre, and the exec depends on the file.
This is one reason why I wanted to restrict the stages to just classes - it
would at least discourage this ability, if not outright prohibit it.
But in the end, it's probably better to leave the sharp knives lying around
than to limit people here.
Comments?
On May 14, 2010, at 2:08 PM, Michael DeHaan wrote:
> So if I understand this correctly, we can now force an apt-get update
> before all the other apt-commands without explicit requires?
>
> Awesome.
>
> --Michael
>
> On Fri, May 14, 2010 at 5:03 PM, Luke Kanies <[email protected]> wrote:
>> This allows you to specify a run stage for either
>> a class or a resource.
>>
>> 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 }
>>
>> If you set a stage on an individual resource, it will
>> break its normal containment relationship. For instance:
>>
>> class foo {
>> file { '/foo': stage => pre }
>> }
>> service { bar: require => Class[foo] }
>>
>> This likely will not behave as you expect, because 'File[/foo]' is no
>> longer in the class foo but has been moved directly into the 'pre' stage,
>> thus losing its containment relationship with Class[foo].
>>
>> Signed-off-by: Luke Kanies <[email protected]>
>> ---
>> lib/puppet/parser/compiler.rb | 26 ++++----
>> lib/puppet/parser/resource.rb | 4 +-
>> lib/puppet/simple_graph.rb | 3 +-
>> lib/puppet/type.rb | 9 +++
>> lib/puppet/type/stage.rb | 7 ++
>> spec/integration/parser/functions/include.rb | 25 --------
>> spec/unit/parser/compiler.rb | 86
>> ++++++++++++++++++++------
>> spec/unit/simple_graph.rb | 8 +++
>> spec/unit/type.rb | 7 ++-
>> spec/unit/type/stage.rb | 9 +++
>> 10 files changed, 123 insertions(+), 61 deletions(-)
>> create mode 100644 lib/puppet/type/stage.rb
>> delete mode 100755 spec/integration/parser/functions/include.rb
>> create mode 100644 spec/unit/type/stage.rb
>>
>> diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
>> index 8e84f5a..9f1acad 100644
>> --- a/lib/puppet/parser/compiler.rb
>> +++ b/lib/puppet/parser/compiler.rb
>> @@ -47,11 +47,14 @@ class Puppet::Parser::Compiler
>> # Note that this will fail if the resource is not unique.
>> @catalog.add_resource(resource)
>>
>> - # And in the resource graph. At some point, this might supercede
>> - # the global resource table, but the table is a lot faster
>> - # so it makes sense to maintain for now.
>> - if resource.type.to_s.downcase == "class" and main =
>> @catalog.resource(:class, :main)
>> - @catalog.add_edge(main, 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" and stage =
>> @catalog.resource(:stage, resource[:stage] || :main)
>> + if resource.type.to_s.downcase == "class" or resource[:stage]
>> + stage = @catalog.resource(:stage, resource[:stage] || :main)
>> + @catalog.add_edge(stage, resource)
>> else
>> @catalog.add_edge(scope.resource, resource)
>> end
>> @@ -161,7 +164,6 @@ class Puppet::Parser::Compiler
>> end
>>
>> initvars()
>> - init_main()
>> end
>>
>> # Create a new scope, with either a specified parent scope or
>> @@ -353,12 +355,6 @@ class Puppet::Parser::Compiler
>> end
>> end
>>
>> - # Initialize the top-level scope, class, and resource.
>> - def init_main
>> - # Create our initial scope and a resource that will evaluate main.
>> - @topscope = Puppet::Parser::Scope.new(:compiler => self)
>> - end
>> -
>> # Set up all of our internal variables.
>> def initvars
>> # The list of objects that will available for export.
>> @@ -378,6 +374,12 @@ class Puppet::Parser::Compiler
>> @catalog = Puppet::Resource::Catalog.new(@node.name)
>> @catalog.version = known_resource_types.version
>>
>> + # Create our initial scope and a resource that will evaluate main.
>> + @topscope = Puppet::Parser::Scope.new(:compiler => self)
>> +
>> + @main_stage_resource = Puppet::Parser::Resource.new("stage", :main,
>> :scope => @topscope)
>> + @catalog.add_resource(@main_stage_resource)
>> +
>> # local resource array to maintain resource ordering
>> @resources = []
>>
>> diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
>> index 7fcb7c1..7b9d0df 100644
>> --- a/lib/puppet/parser/resource.rb
>> +++ b/lib/puppet/parser/resource.rb
>> @@ -22,9 +22,9 @@ class Puppet::Parser::Resource < Puppet::Resource
>> include Puppet::Parser::YamlTrimmer
>>
>> attr_accessor :source, :scope, :rails_id
>> - attr_accessor :virtual, :override, :translated, :catalog
>> + attr_accessor :virtual, :override, :translated, :catalog, :evaluated
>>
>> - attr_reader :exported, :evaluated, :parameters
>> + attr_reader :exported, :parameters
>>
>> # Determine whether the provided parameter name is a relationship
>> parameter.
>> def self.relationship_parameter?(name)
>> diff --git a/lib/puppet/simple_graph.rb b/lib/puppet/simple_graph.rb
>> index 9160394..cf0eff3 100644
>> --- a/lib/puppet/simple_graph.rb
>> +++ b/lib/puppet/simple_graph.rb
>> @@ -323,7 +323,8 @@ class Puppet::SimpleGraph
>> # graph. We could get a similar affect by only setting relationships
>> # to container leaves, but that would result in many more
>> # relationships.
>> - containers = other.topsort.find_all { |v| v.is_a?(type) and
>> vertex?(v) }
>> + stage_class = Puppet::Type.type(:stage)
>> + containers = other.topsort.find_all { |v| (v.is_a?(type) or
>> v.is_a?(stage_class)) and vertex?(v) }
>> containers.each do |container|
>> # Get the list of children from the other graph.
>> children = other.adjacent(container, :direction => :out)
>> diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
>> index 33d2d03..d4aabad 100644
>> --- a/lib/puppet/type.rb
>> +++ b/lib/puppet/type.rb
>> @@ -1402,6 +1402,15 @@ class Type
>> This will restart the sshd service if the sshd config file
>> changes.}
>> end
>>
>> + newmetaparam(:stage) do
>> + desc %{Which run stage a given resource should reside in. This
>> just creates
>> + 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.}
>> +
>> + defaultto :main
>> + end
>> +
>> ###############################
>> # All of the provider plumbing for the resource types.
>> require 'puppet/provider'
>> diff --git a/lib/puppet/type/stage.rb b/lib/puppet/type/stage.rb
>> new file mode 100644
>> index 0000000..5f19b61
>> --- /dev/null
>> +++ b/lib/puppet/type/stage.rb
>> @@ -0,0 +1,7 @@
>> +Puppet::Type.newtype(:stage) do
>> + desc "A resource type for specifying run stages."
>> +
>> + newparam :name do
>> + desc "The name of the stage. This will be used as the 'stage' for
>> each resource."
>> + end
>> +end
>> diff --git a/spec/integration/parser/functions/include.rb
>> b/spec/integration/parser/functions/include.rb
>> deleted file mode 100755
>> index f84d432..0000000
>> --- a/spec/integration/parser/functions/include.rb
>> +++ /dev/null
>> @@ -1,25 +0,0 @@
>> -#!/usr/bin/env ruby
>> -
>> -require File.dirname(__FILE__) + '/../../../spec_helper'
>> -
>> -describe "The include function" do
>> - before :each do
>> - @node = Puppet::Node.new("mynode")
>> - @compiler = Puppet::Parser::Compiler.new(@node)
>> - @compiler.send(:evaluate_main)
>> - @scope = @compiler.topscope
>> - # preload our functions
>> - Puppet::Parser::Functions.function(:include)
>> - Puppet::Parser::Functions.function(:require)
>> - end
>> -
>> - it "should add a containment relationship between the 'included' class
>> and our class" do
>> - @compiler.known_resource_types.add
>> Puppet::Resource::Type.new(:hostclass, "includedclass")
>> -
>> - @scope.function_include("includedclass")
>> -
>> - klass_resource = @compiler.findresource(:class,"includedclass")
>> - klass_resource.should be_instance_of(Puppet::Parser::Resource)
>> - @compiler.catalog.should be_edge(@scope.resource, klass_resource)
>> - end
>> -end
>> diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/compiler.rb
>> index 6fd4d1f..7dd72e8 100755
>> --- a/spec/unit/parser/compiler.rb
>> +++ b/spec/unit/parser/compiler.rb
>> @@ -10,6 +10,10 @@ class CompilerTestResource
>> @title = title
>> end
>>
>> + def [](attr)
>> + :main
>> + end
>> +
>> def ref
>> "%s[%s]" % [type.to_s.capitalize, title]
>> end
>> @@ -31,13 +35,17 @@ class CompilerTestResource
>> end
>>
>> describe Puppet::Parser::Compiler do
>> + def resource(type, title)
>> + Puppet::Parser::Resource.new(type, title, :scope => @scope)
>> + end
>> +
>> before :each do
>> @node = Puppet::Node.new "testnode"
>> @known_resource_types = Puppet::Resource::TypeCollection.new
>> "development"
>>
>> @scope_resource = stub 'scope_resource', :builtin? => true, :finish
>> => nil, :ref => 'Class[main]', :type => "class"
>> - @scope = stub 'scope', :resource => @scope_resource, :source =>
>> mock("source")
>> @compiler = Puppet::Parser::Compiler.new(@node)
>> + @scope = Puppet::Parser::Scope.new(:compiler => @compiler, :source
>> => stub('source'))
>> @compiler.environment.stubs(:known_resource_types).returns
>> @known_resource_types
>> end
>>
>> @@ -99,6 +107,10 @@ describe Puppet::Parser::Compiler do
>> compiler.classlist.should include("foo")
>> compiler.classlist.should include("bar")
>> end
>> +
>> + it "should add a 'main' stage to the catalog" do
>> + @compiler.catalog.resource(:stage, :main).should
>> be_instance_of(Puppet::Parser::Resource)
>> + end
>> end
>>
>> describe "when managing scopes" do
>> @@ -208,7 +220,7 @@ describe Puppet::Parser::Compiler do
>> end
>>
>> it "should ignore builtin resources" do
>> - resource = stub 'builtin', :ref => "File[testing]", :builtin?
>> => true, :type => "file"
>> + resource = resource(:file, "testing")
>>
>> @compiler.add_resource(@scope, resource)
>> resource.expects(:evaluate).never
>> @@ -228,7 +240,9 @@ describe Puppet::Parser::Compiler do
>> end
>>
>> it "should not evaluate already-evaluated resources" do
>> - resource = stub 'already_evaluated', :ref => "File[testing]",
>> :builtin? => false, :evaluated? => true, :virtual? => false, :type => "file"
>> + resource = resource(:file, "testing")
>> + resource.stubs(:evaluated?).returns true
>> +
>> @compiler.add_resource(@scope, resource)
>> resource.expects(:evaluate).never
>>
>> @@ -257,7 +271,7 @@ describe Puppet::Parser::Compiler do
>> @compiler.add_resource(@scope, resource)
>>
>> # And one that does not
>> - dnf = stub "dnf", :ref => "File[dnf]", :type => "file"
>> + dnf = stub "dnf", :ref => "File[dnf]", :type => "file", :[] =>
>> nil
>>
>> @compiler.add_resource(@scope, dnf)
>>
>> @@ -281,16 +295,16 @@ describe Puppet::Parser::Compiler do
>> end
>>
>> it "should return added resources in add order" do
>> - resource1 = stub "1", :ref => "File[yay]", :type => "file"
>> + resource1 = resource(:file, "yay")
>> @compiler.add_resource(@scope, resource1)
>> - resource2 = stub "2", :ref => "File[youpi]", :type => "file"
>> + resource2 = resource(: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 = CompilerTestResource.new(:file, "yay")
>> + resource = resource(:file, "yay")
>> @compiler.add_resource(@scope, resource)
>>
>> @compiler.catalog.should be_vertex(resource)
>> @@ -305,42 +319,74 @@ describe Puppet::Parser::Compiler do
>> end
>>
>> it "should add an edge from the scope resource to the added
>> resource" do
>> - resource = stub "noconflict", :ref => "File[yay]", :type =>
>> "file"
>> + resource = resource(:file, "yay")
>> @compiler.add_resource(@scope, resource)
>>
>> @compiler.catalog.should be_edge(@scope.resource, resource)
>> end
>>
>> - it "should add edges from the class resources to the main class" do
>> - main = CompilerTestResource.new(:class, :main)
>> - @compiler.add_resource(@scope, main)
>> - resource = CompilerTestResource.new(:class, "foo")
>> + it "should add an edge to any specified stage for resources" do
>> + other_stage = resource(:stage, "other")
>> + @compiler.add_resource(@scope, other_stage)
>> + resource = resource(:file, "foo")
>> + resource[:stage] = 'other'
>> +
>> + @compiler.add_resource(@scope, resource)
>> +
>> + @compiler.catalog.edge?(other_stage, resource).should be_true
>> + end
>> +
>> + it "should add edges from the class resources to the main stage if
>> no stage is specified" do
>> + main = @compiler.catalog.resource(:stage, :main)
>> + resource = resource(:class, "foo")
>> @compiler.add_resource(@scope, resource)
>>
>> @compiler.catalog.should be_edge(main, resource)
>> end
>>
>> - it "should just add edges to the scope resource for the class
>> resources when no main class can be found" do
>> - resource = CompilerTestResource.new(:class, "foo")
>> + it "should not add non-class resources that don't specify a stage
>> to the 'main' stage" do
>> + main = @compiler.catalog.resource(:stage, :main)
>> + resource = resource(:file, "foo")
>> @compiler.add_resource(@scope, resource)
>>
>> - @compiler.catalog.should be_edge(@scope.resource, resource)
>> + @compiler.catalog.should_not be_edge(main, resource)
>> + end
>> +
>> + it "should not add any parent-edges to stages" do
>> + stage = resource(:stage, "other")
>> + @compiler.add_resource(@scope, stage)
>> +
>> + @scope.resource = resource(:class, "foo")
>> +
>> + @compiler.catalog.edge?(@scope.resource, stage).should be_false
>> + end
>> +
>> + it "should not attempt to add stages to other stages" do
>> + other_stage = resource(:stage, "other")
>> + second_stage = resource(:stage, "second")
>> + @compiler.add_resource(@scope, other_stage)
>> + @compiler.add_resource(@scope, second_stage)
>> +
>> + second_stage[:stage] = "other"
>> +
>> + @compiler.catalog.edge?(other_stage, second_stage).should
>> be_false
>> end
>>
>> it "should have a method for looking up resources" do
>> - resource = stub 'resource', :ref => "Yay[foo]", :type => "file"
>> + resource = resource(:yay, "foo")
>> @compiler.add_resource(@scope, resource)
>> @compiler.findresource("Yay[foo]").should equal(resource)
>> end
>>
>> it "should be able to look resources up by type and title" do
>> - resource = stub 'resource', :ref => "Yay[foo]", :type => "file"
>> + resource = resource(:yay, "foo")
>> @compiler.add_resource(@scope, resource)
>> @compiler.findresource("Yay", "foo").should equal(resource)
>> end
>>
>> it "should not evaluate virtual defined resources" do
>> - resource = stub 'notevaluated', :ref => "File[testing]",
>> :builtin? => false, :evaluated? => false, :virtual? => true, :type => "file"
>> + resource = resource(:file, "testing")
>> + resource.virtual = true
>> @compiler.add_resource(@scope, resource)
>>
>> resource.expects(:evaluate).never
>> @@ -565,8 +611,8 @@ describe Puppet::Parser::Compiler do
>> describe "when managing resource overrides" do
>>
>> before do
>> - @override = stub 'override', :ref => "My[ref]", :type => "my"
>> - @resource = stub 'resource', :ref => "My[ref]", :builtin? =>
>> true, :type => "my"
>> + @override = stub 'override', :ref => "File[/foo]", :type => "my"
>> + @resource = resource(:file, "/foo")
>> end
>>
>> it "should be able to store overrides" do
>> diff --git a/spec/unit/simple_graph.rb b/spec/unit/simple_graph.rb
>> index f8596c7..234a2fc 100755
>> --- a/spec/unit/simple_graph.rb
>> +++ b/spec/unit/simple_graph.rb
>> @@ -440,6 +440,8 @@ describe Puppet::SimpleGraph do
>> @top = Container.new("top", ["g", "h", @middle, @one, @three])
>> @empty = Container.new("empty", [])
>>
>> + @stage = Puppet::Type.type(:stage).new(:name => "foo")
>> +
>> @contgraph = @top.to_graph
>>
>> # We have to add the container to the main graph, else it won't
>> @@ -477,6 +479,12 @@ describe Puppet::SimpleGraph do
>> @depgraph.vertices.find_all { |v| v.is_a?(Container) }.should
>> be_empty
>> end
>>
>> + # This is a bit hideous, but required to make stages work with
>> relationships - they're
>> + # the top of the graph.
>> + it "should remove all Stage resources from the dependency graph" do
>> + @depgraph.vertices.find_all { |v|
>> v.is_a?(Puppet::Type.type(:stage)) }.should be_empty
>> + end
>> +
>> it "should add container relationships to contained objects" do
>> @contgraph.leaves(@middle).each do |leaf|
>> @depgraph.should be_edge("h", leaf)
>> diff --git a/spec/unit/type.rb b/spec/unit/type.rb
>> index 9381c0a..42d6ae9 100755
>> --- a/spec/unit/type.rb
>> +++ b/spec/unit/type.rb
>> @@ -445,7 +445,12 @@ describe Puppet::Type do
>> end
>> end
>>
>> - describe "when managing relationships" do
>> + it "should have a 'stage' metaparam" do
>> + Puppet::Type.metaparamclass(:stage).should be_instance_of(Class)
>> + end
>> +
>> + it "should default to 'main' for 'stage'" do
>> + Puppet::Type.metaparamclass(:stage).new(:resource =>
>> stub('resource')).default.should == :main
>> end
>> end
>>
>> diff --git a/spec/unit/type/stage.rb b/spec/unit/type/stage.rb
>> new file mode 100644
>> index 0000000..6884652
>> --- /dev/null
>> +++ b/spec/unit/type/stage.rb
>> @@ -0,0 +1,9 @@
>> +#!/usr/bin/env ruby
>> +
>> +require File.dirname(__FILE__) + '/../../spec_helper'
>> +
>> +describe Puppet::Type.type(:stage) do
>> + it "should have a 'name' parameter'" do
>> + Puppet::Type.type(:stage).new(:name => :foo)[:name].should == :foo
>> + end
>> +end
>> --
>> 1.6.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Puppet Developers" group.
>> To post to this group, send email to [email protected].
>> To unsubscribe from this group, send email to
>> [email protected].
>> For more options, visit this group at
>> http://groups.google.com/group/puppet-dev?hl=en.
>>
>>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To post to this group, send email to [email protected].
> To unsubscribe from this group, send email to
> [email protected].
> For more options, visit this group at
> http://groups.google.com/group/puppet-dev?hl=en.
>
--
The real art of conversation is not only to say the right thing at the
right place but to leave unsaid the wrong thing at the tempting
moment. -- Dorothy Nevill
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
