Please review pull request #723: (#13349) Fix incorrect scope behavior opened by (jeffweiss)
Description:
This commit resolves an error with class inclusion, via a resource
rather than the include statement, where if the class was a top-level
class, the class name would get resolved twice: once for adding the
resource (which happened correctly) and again when evaluating the class
(which would incorrectly re-check all the namespaces though with the ::
stripped off) finding the nested version.
Prior to this commit the following manifest would generate both notify
messages. After this commit, the manifest only generates the top-level
notify messages.
class { 'foo::test': }
class foo::test {
class { '::bar::baz': }
}
class bar::baz {
notify { 'good!': }
}
class foo::bar::baz {
notify { 'bad!': }
}
- Opened: Thu Apr 26 21:26:19 UTC 2012
- Based on: puppetlabs:master (d63ca26332d3f93e7cdd1b357e1e246755b21bff)
- Requested merge: jeffweiss:ticket/master/13349_incorrect_scope_behavior (36a7b4b90e01e5ea1054b149e41270ecb137714e)
Diff follows:
diff --git a/acceptance/tests/ticket_13349_single_include_should_not_load_multiple_classes.rb b/acceptance/tests/ticket_13349_single_include_should_not_load_multiple_classes.rb
new file mode 100644
index 0000000..5741a97
--- /dev/null
+++ b/acceptance/tests/ticket_13349_single_include_should_not_load_multiple_classes.rb
@@ -0,0 +1,25 @@
+test_name "class inclusion should respect explicit scoping"
+
+success_message = "GOOD: top level bar::something"
+failure_message = "BAD: nested bar::something in foo"
+
+manifest = %Q[
+ class { 'foo::test': }
+
+ class foo::test {
+ class { '::bar::something': }
+ }
+
+ class bar::something {
+ notify { '#{success_message}': withpath => true }
+ }
+
+ class foo::bar::something {
+ notify { '#{failure_message}': withpath => true }
+ }
+]
+
+on agents, puppet_apply("--verbose"), :stdin => manifest do
+ assert_match(/#{success_message}/, stdout)
+ assert_no_match(/#{failure_message}/, stdout)
+end
diff --git a/lib/puppet/parser/ast/resource.rb b/lib/puppet/parser/ast/resource.rb
index ce3c499..5f5f1ab 100644
--- a/lib/puppet/parser/ast/resource.rb
+++ b/lib/puppet/parser/ast/resource.rb
@@ -57,7 +57,7 @@ def evaluate(scope)
resource.resource_type.instantiate_resource(scope, resource)
end
scope.compiler.add_resource(scope, resource)
- scope.compiler.evaluate_classes([resource_title],scope,false) if fully_qualified_type == 'class'
+ scope.compiler.evaluate_classes([resource_title], scope, false, true) if fully_qualified_type == 'class'
resource
end
}
diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index ec3e7a8..172ffe6 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -135,7 +135,14 @@ def evaluate_node_classes
# find, raise an error. This method really just creates resource objects
# that point back to the classes, and then the resources are themselves
# evaluated later in the process.
- def evaluate_classes(classes, scope, lazy_evaluate = true)
+ #
+ # Sometimes we evaluate classes with a fully qualified name already, in which
+ # case, we tell scope.find_hostclass we've pre-qualified the name so it
+ # doesn't need to search it's namespaces again. This gets around a weird
+ # edge case of duplicate class names, one at top scope and one nested in our
+ # namespace and the wrong one (or both!) getting selected. See ticket #13349
+ # for more detail. --jeffweiss 26 apr 2012
+ def evaluate_classes(classes, scope, lazy_evaluate = true, fqname = false)
raise Puppet::DevError, "No source for scope passed to evaluate_classes" unless scope.source
class_parameters = nil
# if we are a param class, save the classes hash
@@ -146,7 +153,8 @@ def evaluate_classes(classes, scope, lazy_evaluate = true)
end
classes.each do |name|
# If we can find the class, then make a resource that will evaluate it.
- if klass = scope.find_hostclass(name)
+
+ if klass = scope.find_hostclass(name, :assume_fqname => fqname)
# If parameters are passed, then attempt to create a duplicate resource
# so the appropriate error is thrown.
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 0e4df9e..9341a8a 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -115,8 +115,8 @@ def environment
compiler ? compiler.environment : Puppet::Node::Environment.new
end
- def find_hostclass(name)
- known_resource_types.find_hostclass(namespaces, name)
+ def find_hostclass(name, options = {})
+ known_resource_types.find_hostclass(namespaces, name, options)
end
def find_definition(name)
diff --git a/lib/puppet/resource/type_collection.rb b/lib/puppet/resource/type_collection.rb
index 082a11c..8306b77 100644
--- a/lib/puppet/resource/type_collection.rb
+++ b/lib/puppet/resource/type_collection.rb
@@ -108,8 +108,8 @@ def find_node(namespaces, name)
@nodes[munge_name(name)]
end
- def find_hostclass(namespaces, name)
- find_or_load(namespaces, name, :hostclass)
+ def find_hostclass(namespaces, name, options = {})
+ find_or_load(namespaces, name, :hostclass, options)
end
def find_definition(namespaces, name)
@@ -191,8 +191,9 @@ def resolve_namespaces(namespaces, name)
# Resolve namespaces and find the given object. Autoload it if
# necessary.
- def find_or_load(namespaces, name, type)
- resolve_namespaces(namespaces, name).each do |fqname|
+ def find_or_load(namespaces, name, type, options = {})
+ searchspace = options[:assume_fqname] ? [name].flatten : resolve_namespaces(namespaces, name)
+ searchspace.each do |fqname|
if result = send(type, fqname) || loader.try_load_fqname(type, fqname)
return result
end
diff --git a/spec/unit/resource/type_collection_spec.rb b/spec/unit/resource/type_collection_spec.rb
index 23590ac..c812fa7 100755
--- a/spec/unit/resource/type_collection_spec.rb
+++ b/spec/unit/resource/type_collection_spec.rb
@@ -309,7 +309,10 @@
it "should only look in the topclass, if the name is qualified" do
@loader.find_hostclass("foo", "::bar").name.should == 'bar'
end
-
+
+ it "should only look in the topclass, if we assume the name is fully qualified" do
+ @loader.find_hostclass("foo", "bar", :assume_fqname => true).name.should == 'bar'
+ end
end
it "should not look in the local scope for classes when the name is qualified" do
@@ -330,7 +333,7 @@
it "should use the 'find_or_load' method to find hostclasses" do
loader = Puppet::Resource::TypeCollection.new("env")
- loader.expects(:find_or_load).with("foo", "bar", :hostclass)
+ loader.expects(:find_or_load).with("foo", "bar", :hostclass, {})
loader.find_hostclass("foo", "bar")
end
-- 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.
