Please review pull request #673: Bug/2.7.x/8174 incorrect warning about deprecated scoping opened by (zaphod42)
Description:
ActiveRecord installed a parent() method that seems to have wreaked
havoc on the inheritance traversing code in the variable lookup code.
This uses a new method that achieves the same end, but doesn't rely as
much on the duck typing (responds to :parent) of the resource.
In addition tests have been added for node inheritance to ensure that
that inheritance heirarchy is followed as we expect. Also tests that
tried to fake out things and now have incorrect assumptions have been
changed to make fewer assumptions about how the system works.
- Opened: Sat Apr 14 00:29:29 UTC 2012
- Based on: puppetlabs:2.7.x (dff6d9d6e9c145254d820bb698e74917c5b943d7)
- Requested merge: zaphod42:bug/2.7.x/8174-incorrect-warning-about-deprecated-scoping (2d3cc63b9d48c4d315361ca4b1db7e05b0ab909a)
Diff follows:
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 46c1f86..569c3c8 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -241,7 +241,7 @@ def twoscope_lookupvar(name, options = {})
table = ephemeral?(name) ? @ephemeral.last : @symtable
if name =~ /^(.*)::(.+)$/
begin
- qualified_scope($1).twoscope_lookupvar($2,options.merge({:origin => nil}))
+ 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}"
@@ -250,10 +250,10 @@ def twoscope_lookupvar(name, options = {})
# 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
+ elsif resource and resource.type == "Class" 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)
+ parent.twoscope_lookupvar(name, options)
else
:undefined
end
diff --git a/spec/unit/parser/functions/create_resources_spec.rb b/spec/unit/parser/functions/create_resources_spec.rb
index 8d4d5d8..c6cfa5a 100755
--- a/spec/unit/parser/functions/create_resources_spec.rb
+++ b/spec/unit/parser/functions/create_resources_spec.rb
@@ -2,18 +2,17 @@
require 'spec_helper'
describe 'function for dynamically creating resources' do
+ def compile_to_catalog(string)
+ Puppet[:code] = string
+ Puppet::Parser::Compiler.compile(Puppet::Node.new('foonode'))
+ end
- def get_scope
- @topscope = Puppet::Parser::Scope.new
- # This is necessary so we don't try to use the compiler to discover our parent.
- @topscope.parent = nil
+ before :each do
@scope = Puppet::Parser::Scope.new
@scope.compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("floppy", :environment => 'production'))
- @scope.parent = @topscope
+ @topscope = @scope.compiler.topscope
@compiler = @scope.compiler
- end
- before :each do
- get_scope
+ @scope.parent = @topscope
Puppet::Parser::Functions.function(:create_resources)
end
@@ -32,6 +31,7 @@ def get_scope
@scope.function_create_resources(['notify', {'test'=>{}}])
end
end
+
describe 'when the caller supplies a name parameter' do
it 'should set the resource name to the value provided' do
Puppet::Parser::Resource.any_instance.expects(:set_parameter).with(:name, 'user_supplied').once
@@ -41,131 +41,164 @@ def get_scope
end
describe 'when creating native types' do
- before :each do
- Puppet[:code]='notify{test:}'
- get_scope
- @scope.resource=Puppet::Parser::Resource.new('class', 't', :scope => @scope)
- end
-
it 'empty hash should not cause resources to be added' do
- @scope.function_create_resources(['file', {}])
- @compiler.catalog.resources.size == 1
+ catalog = compile_to_catalog("create_resources('file', {})")
+ catalog.resources.size.should == 3
end
+
it 'should be able to add' do
- @scope.function_create_resources(['file', {'/etc/foo'=>{'ensure'=>'present'}}])
- @compiler.catalog.resource(:file, "/etc/foo")['ensure'].should == 'present'
+ catalog = compile_to_catalog("create_resources('file', {'/etc/foo'=>{'ensure'=>'present'}})")
+ catalog.resource(:file, "/etc/foo")['ensure'].should == 'present'
end
+
it 'should accept multiple types' do
- type_hash = {}
- type_hash['foo'] = {'message' => 'one'}
- type_hash['bar'] = {'message' => 'two'}
- @scope.function_create_resources(['notify', type_hash])
- @compiler.catalog.resource(:notify, "foo")['message'].should == 'one'
- @compiler.catalog.resource(:notify, "bar")['message'].should == 'two'
+ catalog = compile_to_catalog("create_resources('notify', {'foo'=>{'message'=>'one'}, 'bar'=>{'message'=>'two'}})")
+ catalog.resource(:notify, "foo")['message'].should == 'one'
+ catalog.resource(:notify, "bar")['message'].should == 'two'
end
+
it 'should fail to add non-existing type' do
expect { @scope.function_create_resources(['create-resource-foo', {}]) }.should raise_error(ArgumentError, 'could not create resource of unknown type create-resource-foo')
end
+
it 'should be able to add edges' do
- @scope.function_create_resources(['notify', {'foo'=>{'require' => 'Notify[test]'}}])
- @scope.compiler.compile
- rg = @scope.compiler.catalog.to_ral.relationship_graph
+ catalog = compile_to_catalog("notify { test: }\n create_resources('notify', {'foo'=>{'require'=>'Notify[test]'}})")
+ rg = catalog.to_ral.relationship_graph
test = rg.vertices.find { |v| v.title == 'test' }
foo = rg.vertices.find { |v| v.title == 'foo' }
test.should be
foo.should be
rg.path_between(test,foo).should be
end
+
it 'should account for default values' do
- @scope.function_create_resources(['file', {'/etc/foo'=>{'ensure'=>'present'}, '/etc/baz'=>{'group'=>'food'}}, {'group' => 'bar'}])
- @compiler.catalog.resource(:file, "/etc/foo")['group'].should == 'bar'
- @compiler.catalog.resource(:file, "/etc/baz")['group'].should == 'food'
+ catalog = compile_to_catalog("create_resources('file', {'/etc/foo'=>{'ensure'=>'present'}, '/etc/baz'=>{'group'=>'food'}}, {'group' => 'bar'})")
+ catalog.resource(:file, "/etc/foo")['group'].should == 'bar'
+ catalog.resource(:file, "/etc/baz")['group'].should == 'food'
end
end
describe 'when dynamically creating resource types' do
- before :each do
- Puppet[:code]=
-'define foocreateresource($one){notify{$name: message => $one}}
-notify{test:}
-'
- get_scope
- @scope.resource=Puppet::Parser::Resource.new('class', 't', :scope => @scope)
- Puppet::Parser::Functions.function(:create_resources)
- end
it 'should be able to create defined resoure types' do
- @scope.function_create_resources(['foocreateresource', {'blah'=>{'one'=>'two'}}])
- # still have to compile for this to work...
- # I am not sure if this constraint ruins the tests
- @scope.compiler.compile
- @compiler.catalog.resource(:notify, "blah")['message'].should == 'two'
+ catalog = compile_to_catalog(<<-MANIFEST)
+ define foocreateresource($one) {
+ notify { $name: message => $one }
+ }
+
+ create_resources('foocreateresource', {'blah'=>{'one'=>'two'}})
+ MANIFEST
+ catalog.resource(:notify, "blah")['message'].should == 'two'
end
+
it 'should fail if defines are missing params' do
- @scope.function_create_resources(['foocreateresource', {'blah'=>{}}])
- expect { @scope.compiler.compile }.should raise_error(Puppet::ParseError, 'Must pass one to Foocreateresource[blah] at line 1')
+ expect {
+ compile_to_catalog(<<-MANIFEST)
+ define foocreateresource($one) {
+ notify { $name: message => $one }
+ }
+
+ create_resources('foocreateresource', {'blah'=>{}})
+ MANIFEST
+ }.should raise_error(Puppet::Error, 'Must pass one to Foocreateresource[blah] at line 1 on node foonode')
end
+
it 'should be able to add multiple defines' do
- hash = {}
- hash['blah'] = {'one' => 'two'}
- hash['blaz'] = {'one' => 'three'}
- @scope.function_create_resources(['foocreateresource', hash])
- # still have to compile for this to work...
- # I am not sure if this constraint ruins the tests
- @scope.compiler.compile
- @compiler.catalog.resource(:notify, "blah")['message'].should == 'two'
- @compiler.catalog.resource(:notify, "blaz")['message'].should == 'three'
+ catalog = compile_to_catalog(<<-MANIFEST)
+ define foocreateresource($one) {
+ notify { $name: message => $one }
+ }
+
+ create_resources('foocreateresource', {'blah'=>{'one'=>'two'}, 'blaz'=>{'one'=>'three'}})
+ MANIFEST
+
+ catalog.resource(:notify, "blah")['message'].should == 'two'
+ catalog.resource(:notify, "blaz")['message'].should == 'three'
end
+
it 'should be able to add edges' do
- @scope.function_create_resources(['foocreateresource', {'blah'=>{'one'=>'two', 'require' => 'Notify[test]'}}])
- @scope.compiler.compile
- rg = @scope.compiler.catalog.to_ral.relationship_graph
+ catalog = compile_to_catalog(<<-MANIFEST)
+ define foocreateresource($one) {
+ notify { $name: message => $one }
+ }
+
+ notify { test: }
+
+ create_resources('foocreateresource', {'blah'=>{'one'=>'two', 'require' => 'Notify[test]'}})
+ MANIFEST
+
+ rg = catalog.to_ral.relationship_graph
test = rg.vertices.find { |v| v.title == 'test' }
blah = rg.vertices.find { |v| v.title == 'blah' }
test.should be
blah.should be
- # (Yoda speak like we do)
rg.path_between(test,blah).should be
- @compiler.catalog.resource(:notify, "blah")['message'].should == 'two'
+ catalog.resource(:notify, "blah")['message'].should == 'two'
end
+
it 'should account for default values' do
- @scope.function_create_resources(['foocreateresource', {'blah'=>{}}, {'one' => 'two'}])
- @scope.compiler.compile
- @compiler.catalog.resource(:notify, "blah")['message'].should == 'two'
+ catalog = compile_to_catalog(<<-MANIFEST)
+ define foocreateresource($one) {
+ notify { $name: message => $one }
+ }
+
+ create_resources('foocreateresource', {'blah'=>{}}, {'one' => 'two'})
+ MANIFEST
+
+ catalog.resource(:notify, "blah")['message'].should == 'two'
end
end
+
describe 'when creating classes' do
- before :each do
- Puppet[:code]=
-'class bar($one){notify{test: message => $one}}
-notify{tester:}
-'
- get_scope
- @scope.resource=Puppet::Parser::Resource.new('class', 't', :scope => @scope)
- Puppet::Parser::Functions.function(:create_resources)
- end
it 'should be able to create classes' do
- @scope.function_create_resources(['class', {'bar'=>{'one'=>'two'}}])
- @scope.compiler.compile
- @compiler.catalog.resource(:notify, "test")['message'].should == 'two'
- @compiler.catalog.resource(:class, "bar").should_not be_nil
+ catalog = compile_to_catalog(<<-MANIFEST)
+ class bar($one) {
+ notify { test: message => $one }
+ }
+
+ create_resources('class', {'bar'=>{'one'=>'two'}})
+ MANIFEST
+
+ catalog.resource(:notify, "test")['message'].should == 'two'
+ catalog.resource(:class, "bar").should_not be_nil
end
+
it 'should fail to create non-existing classes' do
- expect { @scope.function_create_resources(['class', {'blah'=>{'one'=>'two'}}]) }.should raise_error(ArgumentError ,'could not find hostclass blah')
+ expect {
+ compile_to_catalog(<<-MANIFEST)
+ create_resources('class', {'blah'=>{'one'=>'two'}})
+ MANIFEST
+ }.should raise_error(Puppet::Error ,'could not find hostclass blah at line 1 on node foonode')
end
+
it 'should be able to add edges' do
- @scope.function_create_resources(['class', {'bar'=>{'one'=>'two', 'require' => 'Notify[tester]'}}])
- @scope.compiler.compile
- rg = @scope.compiler.catalog.to_ral.relationship_graph
+ catalog = compile_to_catalog(<<-MANIFEST)
+ class bar($one) {
+ notify { test: message => $one }
+ }
+
+ notify { tester: }
+
+ create_resources('class', {'bar'=>{'one'=>'two', 'require' => 'Notify[tester]'}})
+ MANIFEST
+
+ rg = catalog.to_ral.relationship_graph
test = rg.vertices.find { |v| v.title == 'test' }
tester = rg.vertices.find { |v| v.title == 'tester' }
test.should be
tester.should be
rg.path_between(tester,test).should be
end
+
it 'should account for default values' do
- @scope.function_create_resources(['class', {'bar'=>{}}, {'one' => 'two'}])
- @scope.compiler.compile
- @compiler.catalog.resource(:notify, "test")['message'].should == 'two'
- @compiler.catalog.resource(:class, "bar").should_not be_nil
+ catalog = compile_to_catalog(<<-MANIFEST)
+ class bar($one) {
+ notify { test: message => $one }
+ }
+
+ create_resources('class', {'bar'=>{}}, {'one' => 'two'})
+ MANIFEST
+
+ catalog.resource(:notify, "test")['message'].should == 'two'
+ catalog.resource(:class, "bar").should_not be_nil
end
end
end
diff --git a/spec/unit/parser/scope_spec.rb b/spec/unit/parser/scope_spec.rb
index 01dfcd3..ff64fb5 100755
--- a/spec/unit/parser/scope_spec.rb
+++ b/spec/unit/parser/scope_spec.rb
@@ -3,7 +3,6 @@
describe Puppet::Parser::Scope do
before :each do
- # This is necessary so we don't try to use the compiler to discover our parent.
@scope = Puppet::Parser::Scope.new
@scope.compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("foo"))
@scope.source = Puppet::Resource::Type.new(:node, :foo)
@@ -290,6 +289,55 @@ class baz {
Puppet.expects(:deprecation_warning).never
end
+ it "finds value define in the inherited node" do
+ expect_the_message_to_be('parent_msg') do <<-MANIFEST
+ $var = "top_msg"
+ node parent {
+ $var = "parent_msg"
+ }
+ node default inherits parent {
+ include foo
+ }
+ class foo {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+ it "finds top scope when the class is included before the node defines the var" do
+ expect_the_message_to_be('top_msg') do <<-MANIFEST
+ $var = "top_msg"
+ node parent {
+ include foo
+ }
+ node default inherits parent {
+ $var = "default_msg"
+ }
+ class foo {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+ it "finds top scope when the class is included before the node defines the var" do
+ expect_the_message_to_be('top_msg') do <<-MANIFEST
+ $var = "top_msg"
+ node parent {
+ include foo
+ }
+ node default inherits parent {
+ $var = "default_msg"
+ }
+ class foo {
+ notify { 'something': message => $var, }
+ }
+ MANIFEST
+ end
+ end
+
+
it "should find values in its local scope" do
expect_the_message_to_be('local_msg') do <<-MANIFEST
node default {
-- 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.
