From 18ca97b0d16fae149a1eab04c520421b5eb38969 Mon Sep 17 00:00:00 2001 From: Dan Bode Date: Sat, 8 Jan 2011 17:18:25 -0600 Subject: (#5045) Adds support to resource/type to also accept a param hash The params are added as attributes when the resource is created. Also, even if the class already exists, if params are passed, we still try to create it. Reviewed-by: Matt Robinson --- lib/puppet/resource/type.rb | 14 +++++++++++--- spec/unit/resource/type_spec.rb | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb index d40adc145..a31795a1b 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -143,18 +143,26 @@ class Puppet::Resource::Type # classes and nodes. No parameters are be supplied--if this is a # parameterized class, then all parameters take on their default # values. - def ensure_in_catalog(scope) + def ensure_in_catalog(scope, attributes=nil) type == :definition and raise ArgumentError, "Cannot create resources for defined resource types" resource_type = type == :hostclass ? :class : :node # Do nothing if the resource already exists; this makes sure we don't # get multiple copies of the class resource, which helps provide the # singleton nature of classes. - if resource = scope.catalog.resource(resource_type, name) + # we should not do this for classes with attributes + # if attributes are passed, we should still try to create the resource + # even if it exists so that we can fail + # this prevents us from being able to combine param classes with include + if resource = scope.catalog.resource(resource_type, name) and !attributes return resource end - resource = Puppet::Parser::Resource.new(resource_type, name, :scope => scope, :source => self) + if attributes + attributes.each do |k,v| + resource.set_parameter(k,v) + end + end instantiate_resource(scope, resource) scope.compiler.add_resource(scope, resource) resource diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb index 7b240bb82..11288f100 100755 --- a/spec/unit/resource/type_spec.rb +++ b/spec/unit/resource/type_spec.rb @@ -601,12 +601,37 @@ describe Puppet::Resource::Type do @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) end + it "should add specified attributes to the resource" do + @top.ensure_in_catalog(@scope, {'one'=>'1', 'two'=>'2'}) + @compiler.catalog.resource(:class, "top")['one'].should == '1' + @compiler.catalog.resource(:class, "top")['two'].should == '2' + end + + it "should not require params for a param class" do + @top.ensure_in_catalog(@scope, {}) + @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) + end + it "should evaluate the parent class if one exists" do @middle.ensure_in_catalog(@scope) @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) end + it "should evaluate the parent class if one exists" do + @middle.ensure_in_catalog(@scope, {}) + + @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) + end + + it "should fail if you try to create duplicate class resources" do + othertop = Puppet::Parser::Resource.new(:class, 'top',:source => @source, :scope => @scope ) + # add the same class resource to the catalog + @compiler.catalog.add_resource(othertop) + @compiler.catalog.expects(:resource).with(:class, 'top').returns true + lambda { @top.ensure_in_catalog(@scope, {}) }.should raise_error(Puppet::Resource::Catalog::DuplicateResourceError) + end + it "should fail to evaluate if a parent class is defined but cannot be found" do othertop = Puppet::Resource::Type.new :hostclass, "something", :parent => "yay" @code.add othertop -- cgit From a2036ea693996cb6ba5eb9f8f52fefa843a320a6 Mon Sep 17 00:00:00 2001 From: Dan Bode Date: Sat, 8 Jan 2011 17:34:19 -0600 Subject: (#5045) External node classifiers should be able to specify params for classes It facilitates the support for param classes from the ENC. It adds support for classes to be passed as a hash to the evaluate_classes method. If a hash of classes is specified, it also evaluates duplicates. I also had to convert the hash to an array for tags to be applied correctly. Reviewed-by: Matt Robinson --- lib/puppet/parser/compiler.rb | 21 ++++++++++++--- spec/unit/parser/compiler_spec.rb | 55 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index c60e1d4fb..3134e03d9 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -139,12 +139,23 @@ class Puppet::Parser::Compiler def evaluate_classes(classes, scope, lazy_evaluate = true) raise Puppet::DevError, "No source for scope passed to evaluate_classes" unless scope.source found = [] + param_classes = nil + # if we are a param class, save the classes hash + # and transform classes to be the keys + if classes.class == Hash + param_classes = classes + classes = classes.keys + 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) - found << name and next if scope.class_scope(klass) - resource = klass.ensure_in_catalog(scope) + if param_classes + resource = klass.ensure_in_catalog(scope, param_classes[name] || {}) + else + found << name and next if scope.class_scope(klass) and !param_classes + resource = klass.ensure_in_catalog(scope) + end # If they've disabled lazy evaluation (which the :include function does), # then evaluate our resource immediately. @@ -432,7 +443,11 @@ class Puppet::Parser::Compiler @resources = [] # Make sure any external node classes are in our class list - @catalog.add_class(*@node.classes) + if @node.classes.class == Hash + @catalog.add_class(*@node.classes.keys) + else + @catalog.add_class(*@node.classes) + end end # Set the node's parameters into the top-scope as variables. diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index 95f3853e2..c1e7c48c2 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -109,6 +109,15 @@ describe Puppet::Parser::Compiler do compiler.classlist.should include("bar") end + it "should transform node class hashes into a class list" do + node = Puppet::Node.new("mynode") + node.classes = {'foo'=>{'one'=>'1'}, 'bar'=>{'two'=>'2'}} + compiler = Puppet::Parser::Compiler.new(node) + + compiler.classlist.should include("foo") + compiler.classlist.should include("bar") + end + it "should add a 'main' stage to the catalog" do @compiler.catalog.resource(:stage, :main).should be_instance_of(Puppet::Parser::Resource) end @@ -185,6 +194,14 @@ describe Puppet::Parser::Compiler do @compiler.class.publicize_methods(:evaluate_node_classes) { @compiler.evaluate_node_classes } end + it "should evaluate any parameterized classes named in the node" do + classes = {'foo'=>{'1'=>'one'}, 'bar'=>{'2'=>'two'}} + @node.stubs(:classes).returns(classes) + @compiler.expects(:evaluate_classes).with(classes, @compiler.topscope) + @compiler.compile + end + + it "should evaluate the main class if it exists" do compile_stub(:evaluate_main) main_class = @known_resource_types.add Puppet::Resource::Type.new(:hostclass, "") @@ -566,6 +583,14 @@ describe Puppet::Parser::Compiler do @scope.expects(:find_hostclass).with("notfound").returns(nil) @compiler.evaluate_classes(%w{notfound}, @scope) end + # I wish it would fail + it "should log when it can't find class" do + klasses = {'foo'=>nil} + @node.classes = klasses + @compiler.topscope.stubs(:find_hostclass).with('foo').returns(nil) + Puppet.expects(:info).with('Could not find class foo for testnode') + @compiler.compile + end end describe "when evaluating found classes" do @@ -586,6 +611,28 @@ describe Puppet::Parser::Compiler do @compiler.evaluate_classes(%w{myclass}, @scope) end + # test that ensure_in_catalog is called for existing classes + it "should ensure each node class hash is in catalog" do + klasses = {'foo'=>{'1'=>'one'}, 'bar'=>{'2'=>'two'}} + @node.classes = klasses + klasses.each do |name, params| + klass = Puppet::Resource::Type.new(:hostclass, name) + @compiler.topscope.stubs(:find_hostclass).with(name).returns(klass) + klass.expects(:ensure_in_catalog).with(@compiler.topscope, params) + end + @compiler.compile + end + + it "should ensure class is in catalog without params" do + @node.classes = klasses = {'foo'=>nil} + foo = Puppet::Resource::Type.new(:hostclass, 'foo') + @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo) + foo.expects(:ensure_in_catalog).with(@compiler.topscope, {}) + @compiler.compile + end + + + it "should not evaluate the resources created for found classes unless asked" do @compiler.catalog.stubs(:tag) @@ -620,6 +667,14 @@ describe Puppet::Parser::Compiler do @compiler.evaluate_classes(%w{myclass}, @scope, false) end + it "should not skip param classes that have already been evaluated" do + @scope.stubs(:class_scope).with(@class).returns("something") + @node.classes = klasses = {'foo'=>nil} + foo = Puppet::Resource::Type.new(:hostclass, 'foo') + @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo) + foo.expects(:ensure_in_catalog).with(@compiler.topscope, {}) + @compiler.compile + end it "should skip classes previously evaluated with different capitalization" do @compiler.catalog.stubs(:tag) @scope.stubs(:find_hostclass).with("MyClass").returns(@class) -- cgit From 3cfbd0722c5f64a3ef39a65f53fa4195135e90b4 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Fri, 21 Jan 2011 14:07:32 -0800 Subject: (#5045) Cleaning up some tests and code Renamed some variables to be clearer, made tests use less stubbing, added some additional tests and got rid of some unecessary logic. Paired-with: Dan Bode --- lib/puppet/parser/compiler.rb | 2 +- lib/puppet/resource/type.rb | 12 +++--- spec/unit/parser/compiler_spec.rb | 84 ++++++++++++++++++++++++++------------- spec/unit/resource/type_spec.rb | 11 +++-- 4 files changed, 69 insertions(+), 40 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index 3134e03d9..fdabd05c9 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -153,7 +153,7 @@ class Puppet::Parser::Compiler if param_classes resource = klass.ensure_in_catalog(scope, param_classes[name] || {}) else - found << name and next if scope.class_scope(klass) and !param_classes + found << name and next if scope.class_scope(klass) resource = klass.ensure_in_catalog(scope) end diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb index a31795a1b..92e1ef719 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -143,23 +143,23 @@ class Puppet::Resource::Type # classes and nodes. No parameters are be supplied--if this is a # parameterized class, then all parameters take on their default # values. - def ensure_in_catalog(scope, attributes=nil) + def ensure_in_catalog(scope, parameters=nil) type == :definition and raise ArgumentError, "Cannot create resources for defined resource types" resource_type = type == :hostclass ? :class : :node # Do nothing if the resource already exists; this makes sure we don't # get multiple copies of the class resource, which helps provide the # singleton nature of classes. - # we should not do this for classes with attributes - # if attributes are passed, we should still try to create the resource + # we should not do this for classes with parameters + # if parameters are passed, we should still try to create the resource # even if it exists so that we can fail # this prevents us from being able to combine param classes with include - if resource = scope.catalog.resource(resource_type, name) and !attributes + if resource = scope.catalog.resource(resource_type, name) and !parameters return resource end resource = Puppet::Parser::Resource.new(resource_type, name, :scope => scope, :source => self) - if attributes - attributes.each do |k,v| + if parameters + parameters.each do |k,v| resource.set_parameter(k,v) end end diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index c1e7c48c2..687f2ecb9 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -105,8 +105,7 @@ describe Puppet::Parser::Compiler do node.classes = %w{foo bar} compiler = Puppet::Parser::Compiler.new(node) - compiler.classlist.should include("foo") - compiler.classlist.should include("bar") + compiler.classlist.should =~ ['foo', 'bar'] end it "should transform node class hashes into a class list" do @@ -114,8 +113,7 @@ describe Puppet::Parser::Compiler do node.classes = {'foo'=>{'one'=>'1'}, 'bar'=>{'two'=>'2'}} compiler = Puppet::Parser::Compiler.new(node) - compiler.classlist.should include("foo") - compiler.classlist.should include("bar") + compiler.classlist.should =~ ['foo', 'bar'] end it "should add a 'main' stage to the catalog" do @@ -549,7 +547,7 @@ describe Puppet::Parser::Compiler do @compiler.add_collection(coll) - lambda { @compiler.compile }.should raise_error(Puppet::ParseError) + lambda { @compiler.compile }.should raise_error Puppet::ParseError, 'Failed to realize virtual resources something' end it "should fail when there are unevaluated resource collections that refer to multiple specific resources" do @@ -558,7 +556,7 @@ describe Puppet::Parser::Compiler do @compiler.add_collection(coll) - lambda { @compiler.compile }.should raise_error(Puppet::ParseError) + lambda { @compiler.compile }.should raise_error Puppet::ParseError, 'Failed to realize virtual resources one, two' end end @@ -611,28 +609,68 @@ describe Puppet::Parser::Compiler do @compiler.evaluate_classes(%w{myclass}, @scope) end - # test that ensure_in_catalog is called for existing classes - it "should ensure each node class hash is in catalog" do - klasses = {'foo'=>{'1'=>'one'}, 'bar'=>{'2'=>'two'}} + it "should ensure each node class hash is in catalog and have appropriate parameters" do + klasses = {'foo'=>{'1'=>'one'}, 'bar::foo'=>{'2'=>'two'}, 'bar'=>{'1'=> [1,2,3], '2'=>{'foo'=>'bar'}}} @node.classes = klasses + ast_obj = Puppet::Parser::AST::String.new(:value => 'foo') klasses.each do |name, params| - klass = Puppet::Resource::Type.new(:hostclass, name) - @compiler.topscope.stubs(:find_hostclass).with(name).returns(klass) - klass.expects(:ensure_in_catalog).with(@compiler.topscope, params) + klass = Puppet::Resource::Type.new(:hostclass, name, :arguments => {'1' => ast_obj, '2' => ast_obj}) + @compiler.topscope.known_resource_types.add klass end - @compiler.compile + catalog = @compiler.compile + catalog.classes.should =~ ['foo', 'bar::foo', 'settings', 'bar'] + + r1 = catalog.resources.detect {|r| r.title == 'Foo' } + r1.to_hash.should == {:'1' => 'one', :'2' => 'foo'} + r1.tags. should =~ ['class', 'foo'] + + r2 = catalog.resources.detect {|r| r.title == 'Bar::Foo' } + r2.to_hash.should == {:'1' => 'foo', :'2' => 'two'} + r2.tags.should =~ ['bar::foo', 'class', 'bar', 'foo'] + + r2 = catalog.resources.detect {|r| r.title == 'Bar' } + r2.to_hash.should == {:'1' => [1,2,3], :'2' => {'foo'=>'bar'}} + r2.tags.should =~ ['class', 'bar'] + end + + it "should ensure each node class is in catalog and has appropriate tags" do + klasses = ['bar::foo'] + @node.classes = klasses + ast_obj = Puppet::Parser::AST::String.new(:value => 'foo') + klasses.each do |name| + klass = Puppet::Resource::Type.new(:hostclass, name, :arguments => {'1' => ast_obj, '2' => ast_obj}) + @compiler.topscope.known_resource_types.add klass + end + catalog = @compiler.compile + + r2 = catalog.resources.detect {|r| r.title == 'Bar::Foo' } + r2.tags.should =~ ['bar::foo', 'class', 'bar', 'foo'] + end + + it "should fail if required parameters are missing" do + klass = {'foo'=>{'1'=>'one'}} + @node.classes = klass + klass = Puppet::Resource::Type.new(:hostclass, 'foo', :arguments => {'1' => nil, '2' => nil}) + @compiler.topscope.known_resource_types.add klass + lambda { @compiler.compile }.should raise_error Puppet::ParseError, "Must pass 2 to Class[Foo]" + end + + it "should fail if invalid parameters are passed" do + klass = {'foo'=>{'3'=>'one'}} + @node.classes = klass + klass = Puppet::Resource::Type.new(:hostclass, 'foo', :arguments => {'1' => nil, '2' => nil}) + @compiler.topscope.known_resource_types.add klass + lambda { @compiler.compile }.should raise_error Puppet::ParseError, "Invalid parameter 3" end it "should ensure class is in catalog without params" do @node.classes = klasses = {'foo'=>nil} foo = Puppet::Resource::Type.new(:hostclass, 'foo') - @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo) - foo.expects(:ensure_in_catalog).with(@compiler.topscope, {}) - @compiler.compile + @compiler.topscope.known_resource_types.add foo + catalog = @compiler.compile + catalog.classes.should include 'foo' end - - it "should not evaluate the resources created for found classes unless asked" do @compiler.catalog.stubs(:tag) @@ -667,14 +705,6 @@ describe Puppet::Parser::Compiler do @compiler.evaluate_classes(%w{myclass}, @scope, false) end - it "should not skip param classes that have already been evaluated" do - @scope.stubs(:class_scope).with(@class).returns("something") - @node.classes = klasses = {'foo'=>nil} - foo = Puppet::Resource::Type.new(:hostclass, 'foo') - @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo) - foo.expects(:ensure_in_catalog).with(@compiler.topscope, {}) - @compiler.compile - end it "should skip classes previously evaluated with different capitalization" do @compiler.catalog.stubs(:tag) @scope.stubs(:find_hostclass).with("MyClass").returns(@class) @@ -814,7 +844,7 @@ describe Puppet::Parser::Compiler do it "should fail if the compile is finished and resource overrides have not been applied" do @compiler.add_override(@override) - lambda { @compiler.compile }.should raise_error(Puppet::ParseError) + lambda { @compiler.compile }.should raise_error Puppet::ParseError, 'Could not find resource(s) File[/foo] for overriding' end end end diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb index 11288f100..206616ef4 100755 --- a/spec/unit/resource/type_spec.rb +++ b/spec/unit/resource/type_spec.rb @@ -496,7 +496,7 @@ describe Puppet::Resource::Type do it "should evaluate the parent's resource" do @type.parent_type(@scope) - + @type.evaluate_code(@resource) @scope.class_scope(@parent_type).should_not be_nil @@ -504,7 +504,7 @@ describe Puppet::Resource::Type do it "should not evaluate the parent's resource if it has already been evaluated" do @parent_resource.evaluate - + @type.parent_type(@scope) @parent_resource.expects(:evaluate).never @@ -545,7 +545,7 @@ describe Puppet::Resource::Type do it "should not evaluate the parent's resource if it has already been evaluated" do @parent_resource.evaluate - + @type.parent_type(@scope) @parent_resource.expects(:evaluate).never @@ -575,7 +575,7 @@ describe Puppet::Resource::Type do @code = Puppet::Resource::TypeCollection.new("env") @code.add @top @code.add @middle - + @node.environment.stubs(:known_resource_types).returns(@code) end @@ -601,7 +601,7 @@ describe Puppet::Resource::Type do @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) end - it "should add specified attributes to the resource" do + it "should add specified parameters to the resource" do @top.ensure_in_catalog(@scope, {'one'=>'1', 'two'=>'2'}) @compiler.catalog.resource(:class, "top")['one'].should == '1' @compiler.catalog.resource(:class, "top")['two'].should == '2' @@ -628,7 +628,6 @@ describe Puppet::Resource::Type do othertop = Puppet::Parser::Resource.new(:class, 'top',:source => @source, :scope => @scope ) # add the same class resource to the catalog @compiler.catalog.add_resource(othertop) - @compiler.catalog.expects(:resource).with(:class, 'top').returns true lambda { @top.ensure_in_catalog(@scope, {}) }.should raise_error(Puppet::Resource::Catalog::DuplicateResourceError) end -- cgit