Please review pull request #657: Bug/2.7.x/8174 incorrect warning about deprecated scoping opened by (zaphod42)
Description:
This is a slightly modified version of Hunner's pull request (https://github.com/puppetlabs/puppet/pull/604) that fixes a single issue related to not warning in a case involving boolean values and adds some more tests. This appears to fix issue #8174 as well.
- Opened: Wed Apr 11 21:34:24 UTC 2012
- Based on: puppetlabs:2.7.x (868d648524c52905c9a32cecf20d02dd0620d018)
- Requested merge: zaphod42:bug/2.7.x/8174-incorrect-warning-about-deprecated-scoping (9aa302fdcae20353675518e62e773e27ba2ae50c)
Diff follows:
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 746cfbd..46c1f86 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,50 @@ 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 = {})
+ dynamic_value = dynamic_lookupvar(name,options)
+ twoscope_value = twoscope_lookupvar(name,options)
+ if dynamic_value != twoscope_value
+ 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
+ dynamic_value
+ end
+
+ # Look up a variable. The simplest value search we do.
+ def twoscope_lookupvar(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).twoscope_lookupvar($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 resource and resource.resource_type and resource.resource_type.respond_to?("parent") and parent_type = resource.resource_type.parent
+ class_scope(parent_type).twoscope_lookupvar(name,options.merge({:origin => nil}))
+ elsif parent
+ parent.twoscope_lookupvar(name,options)
+ else
+ :undefined
+ end
+ end
+
+ # Look up a variable. The simplest value search we do.
+ def dynamic_lookupvar(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).dynamic_lookupvar($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 +273,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.dynamic_lookupvar(name,options)
else
:undefined
end
@@ -374,7 +407,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..01dfcd3 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
@@ -204,6 +214,261 @@ def create_class_scope(name)
end
end
+ describe "when mixing inheritence and inclusion" do
+ let(:catalog) { Puppet::Parser::Compiler.compile(Puppet::Node.new('foonode')) }
+
+ def expect_the_message_to_be(message)
+ Puppet[:code] = yield
+ catalog = Puppet::Parser::Compiler.compile(Puppet::Node.new('foonode'))
+ catalog.resource('Notify', 'something')[:message].should == message
+ end
+
+ context "deprecated scoping" do
+ before :each do
+ Puppet.expects(:deprecation_warning)
+ end
+
+ it "prefers values in its included scope over those from the node (DEPRECATED)" do
+ expect_the_message_to_be('baz_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include foo
+ }
+ class baz {
+ $var = "baz_msg"
+ include bar
+ }
+ class foo inherits baz {
+ }
+ class bar {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+ it "finds values in its included scope (DEPRECATED)" do
+ expect_the_message_to_be('baz_msg') do <<-MANIFEST
+ node default {
+ include baz
+ }
+ class foo {
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ $var = "baz_msg"
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "recognizes a dynamically scoped boolean (DEPRECATED)" do
+ expect_the_message_to_be(true) do <<-MANIFEST
+ node default {
+ $var = false
+ include baz
+ }
+ class foo {
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ $var = true
+ include bar
+ }
+ MANIFEST
+ end
+ end
+ end
+
+ context "supported scoping" do
+ before :each do
+ Puppet.expects(:deprecation_warning).never
+ end
+
+ it "should find values in its local scope" do
+ expect_the_message_to_be('local_msg') do <<-MANIFEST
+ node default {
+ include baz
+ }
+ class foo {
+ }
+ class bar inherits foo {
+ $var = "local_msg"
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "should find values in its inherited scope" do
+ expect_the_message_to_be('foo_msg') do <<-MANIFEST
+ node default {
+ include baz
+ }
+ class foo {
+ $var = "foo_msg"
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "prefers values in its inherited scope over those in the node (with intermediate inclusion)" do
+ expect_the_message_to_be('foo_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include baz
+ }
+ class foo {
+ $var = "foo_msg"
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "prefers values in its inherited scope over those in the node (without intermediate inclusion)" do
+ expect_the_message_to_be('foo_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include bar
+ }
+ class foo {
+ $var = "foo_msg"
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+ it "prefers values in its inherited scope over those from where it is included" do
+ expect_the_message_to_be('foo_msg') do <<-MANIFEST
+ node default {
+ include baz
+ }
+ class foo {
+ $var = "foo_msg"
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ $var = "baz_msg"
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "does not used variables from classes included in the inherited scope" do
+ expect_the_message_to_be('node_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include bar
+ }
+ class quux {
+ $var = "quux_msg"
+ }
+ class foo inherits quux {
+ }
+ class baz {
+ include foo
+ }
+ class bar inherits baz {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+ it "does not use a variable from a scope lexically enclosing it" do
+ expect_the_message_to_be('node_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include other::bar
+ }
+ class other {
+ $var = "other_msg"
+ class bar {
+ notify { 'something': message => $var, }
+ }
+ }
+ MANIFEST
+ end
+ end
+
+ it "finds values in its node scope" do
+ expect_the_message_to_be('node_msg') do <<-MANIFEST
+ node default {
+ $var = "node_msg"
+ include baz
+ }
+ class foo {
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "finds values in its top scope" do
+ expect_the_message_to_be('top_msg') do <<-MANIFEST
+ $var = "top_msg"
+ node default {
+ include baz
+ }
+ class foo {
+ }
+ class bar inherits foo {
+ notify { 'something': message => $var, }
+ }
+ class baz {
+ include bar
+ }
+ MANIFEST
+ end
+ end
+
+ it "prefers variables from the node over those in the top scope" do
+ expect_the_message_to_be('node_msg') do <<-MANIFEST
+ $var = "top_msg"
+ node default {
+ $var = "node_msg"
+ include foo
+ }
+ class foo {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+ end
+ end
+
describe "when setvar is called with append=true" do
it "should raise error if the variable is already defined in this scope" do
@scope.setvar("var","1", :append => false)
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 [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.
