Bug/2.7.x/8174 incorrect warning about deprecated scoping #673

Merged
merged 3 commits into from Apr 16, 2012

Conversation

Projects
None yet
2 participants
Contributor

zaphod42 commented Apr 14, 2012

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.

Fixing problem caused by activerecord
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.

D'oh.. just noticed I added the comments to the commit, not the pull request... moving...

@@ -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}))
@jeffweiss

jeffweiss Apr 14, 2012

Contributor

Thanks for adding the space... it drove @pcarlisle and me nuts.

@@ -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
@jeffweiss

jeffweiss Apr 14, 2012

Contributor

Would it make sense to add some information within the resource that lets us know if it can have a scope? That way if/when we have something other than Class that has a scope, this code will work properly?

@zaphod42

zaphod42 Apr 16, 2012

Contributor

This is central to the issue that this bug is part of, namely how messed up how concept of scope and variable lookup is. My next task is to work out some other oddities related to scope ($::var can never refer to a var defined in a node and so there is no qualified name for those vars).

I'll have to figure out what resource actually is here in order to add anything to it :)

+ def compile_to_catalog(string)
+ Puppet[:code] = string
+ Puppet::Parser::Compiler.compile(Puppet::Node.new('foonode'))
+ end
@jeffweiss

jeffweiss Apr 14, 2012

Contributor

Does it make sense to move this to a compiler spec helper? it seems like this is valuable enough to use in other places.

- @scope.function_create_resources(['file', {}])
- @compiler.catalog.resources.size == 1
+ catalog = compile_to_catalog("create_resources('file', {})")
+ catalog.resources.size.should == 3
@jeffweiss

jeffweiss Apr 14, 2012

Contributor

Wait, why should the size be 3? We're creating one resource and expecting that it's not being added. That seems a bit magical.
Do we have a test that specifies that by default we expect the catalog to have 3 implicit resources? And if we do, what are they?
If we have an implicit size for the one added, I'd like to see it moved out to a var somewhere @implicit_resources_size perhaps.

Also, if we're checking size, we should have a test that checks the size of the catalog resources after one gets added, @implicit_resources_size + 1.

@zaphod42

zaphod42 Apr 16, 2012

Contributor

A much better assertion is that there is no File resource. Depending on some magic number just doesn't strike me as the right thing here.

zaphod42 added some commits Apr 16, 2012

Cleaner test for create_resources doing nothing
Better expresses that a create_resources with an empty hash is the same
as doing nothing in the catalog at all
Removed duplication of compiling a catalog
Created a PuppetSpec::Compiler module for helping out testing things
that want to compile some code to a catalog
Contributor

jeffweiss commented Apr 16, 2012

Running spec & unit tests in minimal gemset now

Contributor

jeffweiss commented Apr 16, 2012

Passed w/ minimal gemset, starting full gemset tests

@jeffweiss jeffweiss merged commit 0d9e852 into puppetlabs:2.7.x Apr 16, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment