This is again done to make support for hiera easier. The way we were handling lookup of resource defaults was over-complicated.
This does a decent bit of cleanup overall, but primarily focused on resource type defaults and how they get set during compilation. Signed-off-by: Luke Kanies <[email protected]> --- Local-branch: refactor/master/8232-array_indexers_on_scope lib/puppet/parser/compiler.rb | 10 ++++-- lib/puppet/resource.rb | 33 ++++++++++++++++++++ lib/puppet/resource/type.rb | 34 +++++++++----------- spec/unit/parser/compiler_spec.rb | 6 ++-- spec/unit/resource/type_spec.rb | 38 +++++++++++------------ spec/unit/resource_spec.rb | 61 +++++++++++++++++++++++++++++++++++++ 6 files changed, 136 insertions(+), 46 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index f43a312..06cd80a 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -139,19 +139,21 @@ class Puppet::Parser::Compiler # evaluated later in the process. def evaluate_classes(classes, scope, lazy_evaluate = true) raise Puppet::DevError, "No source for scope passed to evaluate_classes" unless scope.source - param_classes = nil + class_parameters = 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 + class_parameters = 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) - if param_classes - resource = klass.ensure_in_catalog(scope, param_classes[name] || {}) + # If parameters are passed, then attempt to create a duplicate resource + # so the appropriate error is thrown. + if class_parameters + resource = klass.ensure_in_catalog(scope, class_parameters[name] || {}) else next if scope.class_scope(klass) resource = klass.ensure_in_catalog(scope) diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb index 59e387d..217eb11 100644 --- a/lib/puppet/resource.rb +++ b/lib/puppet/resource.rb @@ -343,6 +343,26 @@ class Puppet::Resource [ type, title ].join('/') end + def set_default_parameters(scope) + return [] unless resource_type and resource_type.respond_to?(:arguments) + + result = [] + + resource_type.arguments.each do |param, default| + param = param.to_sym + next if parameters.include?(param) + unless is_a?(Puppet::Parser::Resource) + fail Puppet::DevError, "Cannot evaluate default parameters for #{self} - not a parser resource" + end + + next if default.nil? + + self[param] = default.safeevaluate(scope) + result << param + end + result + end + def to_resource self end @@ -351,6 +371,19 @@ class Puppet::Resource resource_type.valid_parameter?(name) end + # Verify that all required arguments are either present or + # have been provided with defaults. + # Must be called after 'set_default_parameters'. We can't join the methods + # because Type#set_parameters needs specifically ordered behavior. + def validate_complete + return unless resource_type and resource_type.respond_to?(:arguments) + + resource_type.arguments.each do |param, default| + param = param.to_sym + fail Puppet::ParseError, "Must pass #{param} to #{self}" unless parameters.include?(param) + end + end + def validate_parameter(name) raise ArgumentError, "Invalid parameter #{name}" unless valid_parameter?(name) end diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb index 7b251e8..ca6e8b5 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -158,11 +158,7 @@ class Puppet::Resource::Type return resource end resource = Puppet::Parser::Resource.new(resource_type, name, :scope => scope, :source => self) - if parameters - parameters.each do |k,v| - resource.set_parameter(k,v) - end - end + assign_parameter_values(parameters, resource) instantiate_resource(scope, resource) scope.compiler.add_resource(scope, resource) resource @@ -188,6 +184,14 @@ class Puppet::Resource::Type @name.is_a?(Regexp) end + def assign_parameter_values(parameters, resource) + return unless parameters + scope = resource.scope || {} + arguments.merge(parameters).each do |name, default| + resource.set_parameter name, default + end + end + # MQR TODO: # # The change(s) introduced by the fix for #4270 are mostly silly & should be @@ -243,22 +247,14 @@ class Puppet::Resource::Type scope["caller_module_name"] = caller_name end scope.class_set(self.name,scope) if hostclass? or node? - # Verify that all required arguments are either present or - # have been provided with defaults. - arguments.each do |param, default| - param = param.to_sym - next if set.include?(param) - - # Even if 'default' is a false value, it's an AST value, so this works fine - fail Puppet::ParseError, "Must pass #{param} to #{resource.ref}" unless default - value = default.safeevaluate(scope) - scope[param.to_s] = value - - # Set it in the resource, too, so the value makes it to the client. - resource[param] = value - end + # Evaluate the default parameters, now that all other variables are set + default_params = resource.set_default_parameters(scope) + default_params.each { |param| scope[param.to_s] = resource[param] } + # This has to come after the above parameters so that default values + # can use their values + resource.validate_complete end # Check whether a given argument is valid. diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index 9648e29..06f8044 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -614,15 +614,15 @@ describe Puppet::Parser::Compiler do @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]" + 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}) + klass = Puppet::Resource::Type.new(:hostclass, 'foo', :arguments => {}) @compiler.topscope.known_resource_types.add klass - lambda { @compiler.compile }.should raise_error Puppet::ParseError, "Invalid parameter 3" + lambda { @compiler.compile }.should raise_error(Puppet::ParseError, "Invalid parameter 3") end it "should ensure class is in catalog without params" do diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb index f29c939..416fbac 100755 --- a/spec/unit/resource/type_spec.rb +++ b/spec/unit/resource/type_spec.rb @@ -238,9 +238,10 @@ describe Puppet::Resource::Type do 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 @resource = Puppet::Parser::Resource.new(:foo, "bar", :scope => @scope) - @type = Puppet::Resource::Type.new(:hostclass, "foo") + @type = Puppet::Resource::Type.new(:definition, "foo") + @resource.environment.known_resource_types.add @type end ['module_name', 'name', 'title'].each do |variable| @@ -256,7 +257,7 @@ describe Puppet::Resource::Type do # this test is to clarify a crazy edge case # if you specify these special names as params, the resource # will override the special variables - it "resource should override defaults" do + it "should allow the resource to override defaults" do @type.set_arguments :name => nil @resource[:name] = 'foobar' var = Puppet::Parser::AST::Variable.new({'value' => 'name'}) @@ -293,13 +294,13 @@ describe Puppet::Resource::Type do end it "should evaluate and set its default values as variables for parameters not provided by the resource" do - @type.set_arguments :foo => stub("value", :safeevaluate => "something") + @type.set_arguments :foo => Puppet::Parser::AST::String.new(:value => "something") @type.set_resource_parameters(@resource, @scope) @scope['foo'].should == "something" end it "should set all default values as parameters in the resource" do - @type.set_arguments :foo => stub("value", :safeevaluate => "something") + @type.set_arguments :foo => Puppet::Parser::AST::String.new(:value => "something") @type.set_resource_parameters(@resource, @scope) @@ -308,7 +309,6 @@ describe Puppet::Resource::Type do it "should fail if the resource does not provide a value for a required argument" do @type.set_arguments :foo => nil - @resource.expects(:to_hash).returns({}) lambda { @type.set_resource_parameters(@resource, @scope) }.should raise_error(Puppet::ParseError) end @@ -344,15 +344,14 @@ describe Puppet::Resource::Type do describe "when describing and managing parent classes" do before do - @code = Puppet::Resource::TypeCollection.new("env") + @krt = Puppet::Node::Environment.new.known_resource_types @parent = Puppet::Resource::Type.new(:hostclass, "bar") - @code.add @parent + @krt.add @parent @child = Puppet::Resource::Type.new(:hostclass, "foo", :parent => "bar") - @code.add @child + @krt.add @child - @env = stub "environment", :known_resource_types => @code - @scope = stub "scope", :environment => @env, :namespaces => [""] + @scope = Puppet::Parser::Scope.new end it "should be able to define a parent" do @@ -365,16 +364,16 @@ describe Puppet::Resource::Type do it "should be able to find parent nodes" do parent = Puppet::Resource::Type.new(:node, "bar") - @code.add parent + @krt.add parent child = Puppet::Resource::Type.new(:node, "foo", :parent => "bar") - @code.add child + @krt.add child child.parent_type(@scope).should equal(parent) end it "should cache a reference to the parent type" do - @code.stubs(:hostclass).with("foo::bar").returns nil - @code.expects(:hostclass).with("bar").once.returns @parent + @krt.stubs(:hostclass).with("foo::bar").returns nil + @krt.expects(:hostclass).with("bar").once.returns @parent @child.parent_type(@scope) @child.parent_type end @@ -386,7 +385,7 @@ describe Puppet::Resource::Type do it "should be considered the child of a parent's parent" do @grandchild = Puppet::Resource::Type.new(:hostclass, "baz", :parent => "foo") - @code.add @grandchild + @krt.add @grandchild @child.parent_type(@scope) @grandchild.parent_type(@scope) @@ -396,7 +395,7 @@ describe Puppet::Resource::Type do it "should correctly state when it is not another type's child" do @notchild = Puppet::Resource::Type.new(:hostclass, "baz") - @code.add @notchild + @krt.add @notchild @notchild.should_not be_child_of(@parent) end @@ -406,14 +405,13 @@ describe Puppet::Resource::Type do before do @compiler = Puppet::Parser::Compiler.new(Puppet::Node.new("mynode")) @scope = Puppet::Parser::Scope.new :compiler => @compiler - @resource = Puppet::Parser::Resource.new(:foo, "yay", :scope => @scope) + @resource = Puppet::Parser::Resource.new(:class, "foo", :scope => @scope) # This is so the internal resource lookup works, yo. @compiler.catalog.add_resource @resource - @known_resource_types = stub 'known_resource_types' - @resource.stubs(:known_resource_types).returns @known_resource_types @type = Puppet::Resource::Type.new(:hostclass, "foo") + @resource.environment.known_resource_types.add @type end it "should add hostclass names to the classes list" do diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb index 5c8e8dc..0485bc7 100755 --- a/spec/unit/resource_spec.rb +++ b/spec/unit/resource_spec.rb @@ -272,6 +272,67 @@ describe Puppet::Resource do Puppet::Resource.new("file", "/foo").should_not == Puppet::Resource.new("file", "/f") end + describe "when setting default parameters" do + before do + @scope = Puppet::Parser::Scope.new + end + + it "should fail when asked to set default values and it is not a parser resource" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "default_param", :arguments => {"a" => Puppet::Parser::AST::String.new(:value => "default")}) + ) + resource = Puppet::Resource.new("default_param", "name") + lambda { resource.set_default_parameters(@scope) }.should raise_error(Puppet::DevError) + end + + it "should evaluate and set any default values when no value is provided" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "default_param", :arguments => {"a" => Puppet::Parser::AST::String.new(:value => "a_default_value")}) + ) + resource = Puppet::Parser::Resource.new("default_param", "name", :scope => Puppet::Parser::Scope.new) + resource.set_default_parameters(@scope) + resource["a"].should == "a_default_value" + end + + it "should skip attributes with no default value" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "no_default_param", :arguments => {"a" => Puppet::Parser::AST::String.new(:value => "a_default_value")}) + ) + resource = Puppet::Parser::Resource.new("no_default_param", "name", :scope => Puppet::Parser::Scope.new) + lambda { resource.set_default_parameters(@scope) }.should_not raise_error + end + + it "should return the list of default parameters set" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "default_param", :arguments => {"a" => Puppet::Parser::AST::String.new(:value => "a_default_value")}) + ) + resource = Puppet::Parser::Resource.new("default_param", "name", :scope => Puppet::Parser::Scope.new) + resource.set_default_parameters(@scope).should == [:a] + end + end + + describe "when validating all required parameters are present" do + it "should be able to validate that all required parameters are present" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "required_param", :arguments => {"a" => nil}) + ) + lambda { Puppet::Resource.new("required_param", "name").validate_complete }.should raise_error(Puppet::ParseError) + end + + it "should not fail when all required parameters are present" do + Puppet::Node::Environment.new.known_resource_types.add( + Puppet::Resource::Type.new(:definition, "no_required_param") + ) + resource = Puppet::Resource.new("no_required_param", "name") + resource["a"] = "meh" + lambda { resource.validate_complete }.should_not raise_error + end + + it "should not validate against builtin types" do + lambda { Puppet::Resource.new("file", "/bar").validate_complete }.should_not raise_error + end + end + describe "when referring to a resource with name canonicalization" do it "should canonicalize its own name" do res = Puppet::Resource.new("file", "/path/") -- 1.7.3.1 -- 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.
