Please review pull request #592: Bug/2.7.x/incorrect dynamic scope warnings opened by (hunner)
Description:
Until the scope inheritance is completely fixed for the new non-dynamic scoping model, we should give appropriate dynamic scoping deprecation warnings.
This branch will perform two queries; one with the dynamic scoping and a second one that directly references the local scope or top/node scope. If the answers differ then removing dynamic scoping would affect the puppet run and a warning should be given.
- Opened: Fri Mar 23 22:07:58 UTC 2012
- Based on: puppetlabs:master (0b900c9548f6c652f0d7271156b69c35aa232ead)
- Requested merge: hunner:bug/2.7.x/incorrect_dynamic_scope-warnings (fa2be2691dd1ba65eb871690d16b540c32ec1453)
Diff follows:
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 746cfbd..24b725e 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -21,7 +21,7 @@ class Puppet::Parser::Scope
attr_accessor :source, :resource
attr_accessor :base, :keyword
attr_accessor :top, :translated, :compiler
- attr_accessor :parent, :dynamic
+ attr_accessor :parent
attr_reader :namespaces
# thin wrapper around an ephemeral
@@ -222,13 +222,48 @@ def qualified_scope(classname)
private :qualified_scope
- # Look up a variable. The simplest value search we do.
+ # Look up a variable with traditional scoping and then with new scoping. If
+ # the answers differ then print a deprecation warning.
def lookupvar(name, options = {})
+ oldval = oldlookupvar(name,options)
+ newval = newlookupvar(name,options)
+ if oldval and newval and newval != oldval
+ location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : ''
+ Puppet.deprecation_warning "Dynamic lookup of $#{name}#{location} is deprecated. Support will be removed in a later version of Puppet. Use a fully-qualified variable name (e.g., $classname::variable) or parameterized classes."
+ end
+ oldval
+ end
+
+ # Look up a variable. The simplest value search we do.
+ def newlookupvar(name, options = {})
+ # Save the originating scope for the request
+ options[:origin] = self unless options[:origin]
+ table = ephemeral?(name) ? @ephemeral.last : @symtable
+ if name =~ /^(.*)::(.+)$/
+ begin
+ qualified_scope($1).newlookupvar($2,options.merge({:origin => nil}))
+ rescue RuntimeError => e
+ location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : ''
+ warning "Could not look up qualified variable '#{name}'; #{e.message}#{location}"
+ :undefined
+ end
+ # If the value is present and either we are top/node scope or originating scope...
+ elsif (ephemeral_include?(name) or table.include?(name)) and (compiler and self == compiler.topscope or (self.resource and self.resource.type == "Node") or self == options[:origin])
+ table[name]
+ elsif parent
+ parent.newlookupvar(name,options)
+ else
+ :undefined
+ end
+ end
+
+ # Look up a variable. The simplest value search we do.
+ def oldlookupvar(name, options = {})
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 =~ /^(.*)::(.+)$/
begin
- qualified_scope($1).lookupvar($2,options)
+ qualified_scope($1).oldlookupvar($2,options)
rescue RuntimeError => e
location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : ''
warning "Could not look up qualified variable '#{name}'; #{e.message}#{location}"
@@ -236,13 +271,9 @@ def lookupvar(name, options = {})
end
elsif ephemeral_include?(name) or table.include?(name)
# We can't use "if table[name]" here because the value might be false
- if options[:dynamic] and self != compiler.topscope
- location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : ''
- Puppet.deprecation_warning "Dynamic lookup of $#{name}#{location} is deprecated. Support will be removed in Puppet 2.8. Use a fully-qualified variable name (e.g., $classname::variable) or parameterized classes."
- end
table[name]
elsif parent
- parent.lookupvar(name,options.merge(:dynamic => (dynamic || options[:dynamic])))
+ parent.oldlookupvar(name,options)
else
:undefined
end
@@ -374,7 +405,7 @@ def unset_ephemeral_var(level=:all)
# check if name exists in one of the ephemeral scope.
def ephemeral_include?(name)
- @ephemeral.reverse.each do |eph|
+ @ephemeral.reverse_each do |eph|
return true if eph.include?(name)
end
false
diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb
index 600f449..16ed977 100644
--- a/lib/puppet/resource/type.rb
+++ b/lib/puppet/resource/type.rb
@@ -66,7 +66,7 @@ def evaluate_code(resource)
static_parent = evaluate_parent_type(resource)
scope = static_parent || resource.scope
- scope = scope.newscope(:namespace => namespace, :source => self, :resource => resource, :dynamic => !static_parent) unless resource.title == :main
+ scope = scope.newscope(:namespace => namespace, :source => self, :resource => resource) unless resource.title == :main
scope.compiler.add_class(name) unless definition?
set_resource_parameters(resource, scope)
diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb
index 67f0db0..b36ca6d 100755
--- a/spec/unit/parser/scope_spec.rb
+++ b/spec/unit/parser/scope_spec.rb
@@ -3,11 +3,11 @@
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"))
+ @scope.source = Puppet::Resource::Type.new(:node, :foo)
+ @topscope = @scope.compiler.topscope
@scope.parent = @topscope
end
@@ -92,14 +92,6 @@
Puppet::Parser::Scope.new.singleton_class.ancestors.should be_include(mod)
end
-
- it "should remember if it is dynamic" do
- (!!Puppet::Parser::Scope.new(:dynamic => true).dynamic).should == true
- end
-
- it "should assume it is not dynamic" do
- (!Puppet::Parser::Scope.new.dynamic).should == true
- end
end
describe "when looking up a variable" do
@@ -128,10 +120,20 @@
@scope.lookupvar("var").should == "childval"
end
+ it "should be able to look up intermediary variables in parent scopes (DEPRECATED)" do
+ Puppet.expects(:deprecation_warning)
+ thirdscope = Puppet::Parser::Scope.new
+ thirdscope.parent = @scope
+ thirdscope.source = Puppet::Resource::Type.new(:hostclass, :foo, :module_name => "foo")
+ @scope.source = Puppet::Resource::Type.new(:hostclass, :bar, :module_name => "bar")
+
+ @topscope.setvar("var2","parentval")
+ @scope.setvar("var2","childval")
+ thirdscope.lookupvar("var2").should == "childval"
+ end
+
describe "and the variable is qualified" do
- before do
- @compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("foonode"))
- @scope.compiler = @compiler
+ before :each do
@known_resource_types = @scope.known_resource_types
end
@@ -151,6 +153,7 @@ def create_class_scope(name)
end
it "should be able to look up explicitly fully qualified variables from main" do
+ Puppet.expects(:deprecation_warning).never
other_scope = create_class_scope("")
other_scope.setvar("othervar", "otherval")
@@ -159,6 +162,7 @@ def create_class_scope(name)
end
it "should be able to look up explicitly fully qualified variables from other scopes" do
+ Puppet.expects(:deprecation_warning).never
other_scope = create_class_scope("other")
other_scope.setvar("var", "otherval")
@@ -167,6 +171,7 @@ def create_class_scope(name)
end
it "should be able to look up deeply qualified variables" do
+ Puppet.expects(:deprecation_warning).never
other_scope = create_class_scope("other::deep::klass")
other_scope.setvar("var", "otherval")
@@ -175,28 +180,33 @@ def create_class_scope(name)
end
it "should return ':undefined' for qualified variables that cannot be found in other classes" do
+ Puppet.expects(:deprecation_warning).never
other_scope = create_class_scope("other::deep::klass")
@scope.lookupvar("other::deep::klass::var").should == :undefined
end
it "should warn and return ':undefined' for qualified variables whose classes have not been evaluated" do
+ Puppet.expects(:deprecation_warning).never
klass = newclass("other::deep::klass")
- @scope.expects(:warning)
+ @scope.expects(:warning).at_least_once
@scope.lookupvar("other::deep::klass::var").should == :undefined
end
it "should warn and return ':undefined' for qualified variables whose classes do not exist" do
- @scope.expects(:warning)
+ Puppet.expects(:deprecation_warning).never
+ @scope.expects(:warning).at_least_once
@scope.lookupvar("other::deep::klass::var").should == :undefined
end
it "should return ':undefined' when asked for a non-string qualified variable from a class that does not exist" do
+ Puppet.expects(:deprecation_warning).never
@scope.stubs(:warning)
@scope.lookupvar("other::deep::klass::var").should == :undefined
end
it "should return ':undefined' when asked for a non-string qualified variable from a class that has not been evaluated" do
+ Puppet.expects(:deprecation_warning).never
@scope.stubs(:warning)
klass = newclass("other::deep::klass")
@scope.lookupvar("other::deep::klass::var").should == :undefined
diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb
index 352f767..19f86e7 100755
--- a/spec/unit/resource/type_spec.rb
+++ b/spec/unit/resource/type_spec.rb
@@ -238,7 +238,7 @@ def double_convert
describe "when setting its parameters in the scope" do
before do
- @scope = Puppet::Parser::Scope.new(:compiler => stub("compiler", :environment => Puppet::Node::Environment.new), :source => stub("source"))
+ @scope = Puppet::Parser::Scope.new(:compiler => Puppet::Parser::Compiler.new(Puppet::Node.new("foo")), :source => stub("source"))
@resource = Puppet::Parser::Resource.new(:foo, "bar", :scope => @scope)
@type = Puppet::Resource::Type.new(:hostclass, "foo")
end
@@ -435,7 +435,7 @@ def double_convert
it "should set all of its parameters in a subscope" do
subscope = stub 'subscope', :compiler => @compiler
- @scope.expects(:newscope).with(:source => @type, :dynamic => true, :namespace => 'foo', :resource => @resource).returns subscope
+ @scope.expects(:newscope).with(:source => @type, :namespace => 'foo', :resource => @resource).returns subscope
@type.expects(:set_resource_parameters).with(@resource, subscope)
@type.evaluate_code(@resource)
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
