Puppet (master/apply) will now warn at most once per compile if the manifest being compiled uses dynamic scope. During variable lookup, it will perform both dynamic scope lookup and static scope lookup and compare the two. If they differ, it issues the warning.
Signed-off-by: Nick Lewis <[email protected]> --- lib/puppet/parser/ast.rb | 4 +++ lib/puppet/parser/compiler.rb | 4 ++- lib/puppet/parser/scope.rb | 26 +++++++++++++++++++---- lib/puppet/resource/type.rb | 9 +++++-- spec/unit/parser/compiler_spec.rb | 41 +++++++++---------------------------- spec/unit/parser/scope_spec.rb | 30 +++++++++++++++++++++++--- spec/unit/resource/type_spec.rb | 16 +++++++++----- 7 files changed, 80 insertions(+), 50 deletions(-) diff --git a/lib/puppet/parser/ast.rb b/lib/puppet/parser/ast.rb index 0389116..2cc1464 100644 --- a/lib/puppet/parser/ast.rb +++ b/lib/puppet/parser/ast.rb @@ -66,6 +66,8 @@ class Puppet::Parser::AST # it can enable you to catch the error where it happens, rather than # much higher up the stack. def safeevaluate(*options) + previous_context = Thread.current[:context] + Thread.current[:context] = self # We duplicate code here, rather than using exceptwrap, because this # is called so many times during parsing. begin @@ -78,6 +80,8 @@ class Puppet::Parser::AST # not exceptions. raise adderrorcontext(error, detail) end + ensure + Thread.current[:context] = previous_context end # Initialize the object. Requires a hash as the argument, and diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index e1227e7..e40cb92 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -26,6 +26,7 @@ class Puppet::Parser::Compiler Thread.current[:env_module_directories] = nil end + attr_accessor :issued_scope_warning attr_reader :node, :facts, :collections, :catalog, :node_scope, :resources, :relationships # Add a collection to the global list. @@ -169,6 +170,7 @@ class Puppet::Parser::Compiler def initialize(node, options = {}) @node = node + @issued_scope_warning = false options.each do |param, value| begin @@ -184,7 +186,6 @@ class Puppet::Parser::Compiler # Create a new scope, with either a specified parent scope or # using the top scope. def newscope(parent, options = {}) - parent ||= topscope options[:compiler] = self scope = Puppet::Parser::Scope.new(options) scope.parent = parent @@ -227,6 +228,7 @@ class Puppet::Parser::Compiler # Now set the node scope appropriately, so that :topscope can # behave differently. @node_scope = topscope.class_scope(astnode) + @node_scope.parent_relationship = :inherited end # Evaluate our collections and return true if anything returned an object. diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb index 24f1d01..02ec641 100644 --- a/lib/puppet/parser/scope.rb +++ b/lib/puppet/parser/scope.rb @@ -21,7 +21,7 @@ class Puppet::Parser::Scope attr_accessor :level, :source, :resource attr_accessor :base, :keyword attr_accessor :top, :translated, :compiler - attr_accessor :parent + attr_accessor :parent, :parent_relationship attr_reader :namespaces # thin wrapper around an ephemeral @@ -233,14 +233,26 @@ class Puppet::Parser::Scope private :lookup_qualified_var - # Look up a variable. The simplest value search we do. Default to returning - # an empty string for missing values, but support returning a constant. def lookupvar(name, usestring = true) - table = ephemeral?(name) ? @ephemeral.last : @symtable # If the variable is qualified, then find the specified scope and look the variable up there instead. if name =~ /::/ return lookup_qualified_var(name, usestring) end + dynamic = lookupvar_helper(name, usestring, true) + unless compiler.issued_scope_warning + static = lookupvar_helper(name, usestring, false) + if dynamic != static + warning "Dynamic lookup of $#{name}#{Thread.current[:context].error_context} will not be supported in future versions. Use a fully-qualified variable name or parameterized classes." + compiler.issued_scope_warning = true + end + end + dynamic + end + + # Look up a variable. The simplest value search we do. Default to returning + # an empty string for missing values, but support returning a constant. + def lookupvar_helper(name, usestring = true, dynamic = true) + table = ephemeral?(name) ? @ephemeral.last : @symtable # We can't use "if table[name]" here because the value might be false if ephemeral_include?(name) or table.include?(name) if usestring and table[name] == :undef @@ -249,7 +261,11 @@ class Puppet::Parser::Scope return table[name] end elsif self.parent - return parent.lookupvar(name, usestring) + if dynamic or self.parent_relationship == :inherited + return parent.lookupvar_helper(name, usestring, dynamic) + else + return compiler.topscope.lookupvar_helper(name, usestring, dynamic) + end elsif usestring return "" else diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb index 3edf286..dc39f90 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -66,9 +66,12 @@ class Puppet::Resource::Type if tmp = evaluate_parent_type(resource) scope = tmp + parent_relationship = :inherited + else + parent_relationship = :dynamic end - scope = subscope(scope, resource) unless resource.title == :main + scope = subscope(scope, resource, parent_relationship) unless resource.title == :main scope.compiler.add_class(name) unless definition? set_resource_parameters(resource, scope) @@ -251,8 +254,8 @@ class Puppet::Resource::Type end # Create a new subscope in which to evaluate our code. - def subscope(scope, resource) - scope.newscope :resource => resource, :namespace => self.namespace, :source => self + def subscope(scope, resource, parent_relationship) + scope.newscope :resource => resource, :namespace => self.namespace, :source => self, :parent_relationship => parent_relationship end # Check whether a given argument is valid. diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index 22d52f2..d9dd297 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -134,13 +134,6 @@ describe Puppet::Parser::Compiler do newscope.parent.should equal(scope) end - - it "should set the parent scope of the new scope to its topscope if the parent passed in is nil" do - scope = mock 'scope' - newscope = @compiler.newscope(nil) - - newscope.parent.should equal(@compiler.topscope) - end end describe "when compiling" do @@ -674,52 +667,38 @@ describe Puppet::Parser::Compiler do end it "should evaluate the first node class matching the node name" do - node_class = stub 'node', :name => "c", :evaluate_code => nil + node_class = Puppet::Resource::Type.new(:node, "c") @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) - node_resource = stub 'node resource', :ref => "Node[c]", :evaluate => nil, :type => "node" - node_class.expects(:mk_plain_resource).returns(node_resource) - @compiler.compile + @compiler.node_scope.resource.source.should == node_class + @compiler.node_scope.resource.should be_evaluated end it "should match the default node if no matching node can be found" do - node_class = stub 'node', :name => "default", :evaluate_code => nil + node_class = Puppet::Resource::Type.new(:node, "default") @compiler.known_resource_types.stubs(:node).with("default").returns(node_class) - node_resource = stub 'node resource', :ref => "Node[default]", :evaluate => nil, :type => "node" - node_class.expects(:mk_plain_resource).returns(node_resource) - @compiler.compile + @compiler.node_scope.resource.source.should == node_class + @compiler.node_scope.resource.should be_evaluated end it "should evaluate the node resource immediately rather than using lazy evaluation" do - node_class = stub 'node', :name => "c" + node_class = Puppet::Resource::Type.new(:node, "c") @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) - node_resource = stub 'node resource', :ref => "Node[c]", :type => "node" - node_class.expects(:mk_plain_resource).returns(node_resource) - - node_resource.expects(:evaluate) - @compiler.send(:evaluate_ast_node) + @compiler.node_scope.resource.should be_evaluated end it "should set the node's scope as the top scope" do - node_resource = stub 'node resource', :ref => "Node[c]", :evaluate => nil, :type => "node" - node_class = stub 'node', :name => "c", :mk_plain_resource => node_resource - + node_class = Puppet::Resource::Type.new(:node, "c") @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) - # The #evaluate method normally does this. - scope = stub 'scope', :source => "mysource" - @compiler.topscope.expects(:class_scope).with(node_class).returns(scope) - node_resource.stubs(:evaluate) - @compiler.stubs :create_settings_scope - @compiler.compile - @compiler.topscope.should equal(scope) + @compiler.topscope.should == @compiler.topscope.class_scope(node_class) end end diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb index 2e390a5..425b41b 100755 --- a/spec/unit/parser/scope_spec.rb +++ b/spec/unit/parser/scope_spec.rb @@ -4,11 +4,9 @@ require File.dirname(__FILE__) + '/../../spec_helper' describe Puppet::Parser::Scope do before :each do - @topscope = Puppet::Parser::Scope.new - # This is necessary so we don't try to use the compiler to discover our parent. - @topscope.parent = nil @scope = Puppet::Parser::Scope.new @scope.compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("foo")) + @topscope = @scope.compiler.topscope @scope.parent = @topscope end @@ -76,6 +74,11 @@ describe Puppet::Parser::Scope do end describe "when looking up a variable" do + before :each do + Thread.current[:context] = stub "context", :error_context => " at test:1" + @scope.expects(:warning).never + end + it "should default to an empty string" do @scope.lookupvar("var").should == "" end @@ -98,7 +101,7 @@ describe Puppet::Parser::Scope do @scope.lookupvar("var").should == {"a" => "b"} end - it "should be able to look up variables in parent scopes" do + it "should be able to look up variables in topscope" do @topscope.setvar("var", "parentval") @scope.lookupvar("var").should == "parentval" end @@ -109,6 +112,25 @@ describe Puppet::Parser::Scope do @scope.lookupvar("var").should == "childval" end + it "should issue a warning when dynamic scope is used" do + @parent_scope = @scope.compiler.newscope(@topscope) + @scope.parent = @parent_scope + @scope.parent_relationship = :dynamic + @parent_scope.setvar("var", "parentval") + @topscope.setvar("var", "topscopeval") + @scope.expects(:warning).with {|msg| msg =~ /Dynamic lookup/} + @scope.lookupvar("var").should == "parentval" + end + + it "should not issue a warning when inherited scope is used" do + @parent_scope = @scope.compiler.newscope(@topscope) + @scope.parent = @parent_scope + @scope.parent_relationship = :inherited + @parent_scope.setvar("var", "parentval") + @topscope.setvar("var", "topscopeval") + @scope.lookupvar("var").should == "parentval" + end + describe "and the variable is qualified" do before do @compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("foonode")) diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb index 7701e55..348fbfe 100755 --- a/spec/unit/resource/type_spec.rb +++ b/spec/unit/resource/type_spec.rb @@ -234,23 +234,23 @@ describe Puppet::Resource::Type do it "should return a new scope created with the provided scope as the parent" do @scope.expects(:newscope).returns "foo" - @type.subscope(@scope, @resource).should == "foo" + @type.subscope(@scope, @resource, :dynamic).should == "foo" end it "should set the source as itself" do @scope.expects(:newscope).with { |args| args[:source] == @type } - @type.subscope(@scope, @resource) + @type.subscope(@scope, @resource, :dynamic) end it "should set the scope's namespace to its namespace" do @type.expects(:namespace).returns "yayness" @scope.expects(:newscope).with { |args| args[:namespace] == "yayness" } - @type.subscope(@scope, @resource) + @type.subscope(@scope, @resource, :dynamic) end it "should set the scope's resource to the provided resource" do @scope.expects(:newscope).with { |args| args[:resource] == @resource } - @type.subscope(@scope, @resource) + @type.subscope(@scope, @resource, :dynamic) end end @@ -431,7 +431,7 @@ describe Puppet::Resource::Type do it "should set all of its parameters in a subscope" do subscope = stub 'subscope', :compiler => @compiler - @type.expects(:subscope).with(@scope, @resource).returns subscope + @type.expects(:subscope).with(@scope, @resource, :dynamic).returns subscope @type.expects(:set_resource_parameters).with(@resource, subscope) @type.evaluate_code(@resource) @@ -445,8 +445,10 @@ describe Puppet::Resource::Type do @type.evaluate_code(@resource) end - it "should store the class scope" do + it "should create and store the class scope" do @type.evaluate_code(@resource) + + @scope.class_scope(@type).parent_relationship.should == :dynamic @scope.class_scope(@type).should be_instance_of(@scope.class) end @@ -517,6 +519,7 @@ describe Puppet::Resource::Type do @type.evaluate_code(@resource) + @scope.class_scope(@type).parent_relationship.should == :inherited @scope.class_scope(@type).parent.object_id.should == @scope.class_scope(@parent_type).object_id end end @@ -557,6 +560,7 @@ describe Puppet::Resource::Type do @type.evaluate_code(@resource) + @scope.class_scope(@type).parent_relationship.should == :inherited @scope.class_scope(@type).parent.object_id.should == @scope.class_scope(@parent_type).object_id end end -- 1.7.3.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.
