On Tue, Oct 12, 2010 at 3:21 PM, Paul Berry <[email protected]> wrote:
> On Tue, Oct 12, 2010 at 12:46 PM, Jesse A Wolfe <[email protected]> wrote: > >> # Make an instance of our resource type. This is only possible >>> # for those classes and nodes that don't have any arguments, and is >>> # only useful for things like the 'include' function. >>> >> >> The bit about "don't have any arguments" seems stale, doesn't it? >> > > You're right. I will fix this. > > >> >> >>> - def mk_plain_resource(scope) >>> + def mk_singleton_resource(scope) >>> >> >> I agree that "mk_plain_resource" is not a good name, but I'm not sure if >> "singleton resource" is much clearer - is that a new coinage or are we >> already using it as jargon elsewhere? (I recognize that we've got some >> naming work to do outside the scope of this patch, we need a more >> descriptive word for ["classes" "nodes" and "defined types"] than >> "Resource::Type") >> >> and if you're going to be renaming it anyway, let's lose the "mk". I think >> it stands for "make", but the abbreviation is unnecessary, and "make" is a >> nearly meaningless word in this context. >> >> > The two pieces of information I want to be captured (or at least hinted at) > in the name are the fact at (a) the method both creates the resource and > installs it in the catalog--it's not just an accessor, and (b) the method is > idempotent--it doesn't do anything if the resource has already been created > and installed in the catalog. (a) is what motivated keeping "mk" in the > name, and (b) is what motivated the word "singleton". > > How would you feel about calling it "ensure_in_catalog"? > > >> + if ['Class', 'Node'].include? resource.type >>> + scope.catalog.tag(*resource.tags) >>> + end >> >> >> I'd weakly-prefer this to be extracted into a method, too. >> > > Ok, I don't see any harm in that. I'll fold that change in. > Ok, I made the first two changes (fixing the comment and renaming mk_singleton_resource) and squashed them into the commit. As for the third change (extracting the call to scope.catalog.tag into a method), I had trouble deciding what the method should be called and where it should go, so I talked it over with Jesse, and we decidid to leave it as is. I believe this addresses all the issues we've discussed. You can find the full patch here: http://github.com/stereotype441/puppet/tree/ticket/2.6.x/4534 Squashed changes follow. diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index 00ef000..c60e1d4 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -144,7 +144,7 @@ class Puppet::Parser::Compiler if klass = scope.find_hostclass(name) found << name and next if scope.class_scope(klass) - resource = klass.mk_singleton_resource(scope) + resource = klass.ensure_in_catalog(scope) # If they've disabled lazy evaluation (which the :include function does), # then evaluate our resource immediately. @@ -220,7 +220,7 @@ class Puppet::Parser::Compiler # Create a resource to model this node, and then add it to the list # of resources. - resource = astnode.mk_singleton_resource(topscope) + resource = astnode.ensure_in_catalog(topscope) resource.evaluate diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb index e9e6466..138ce17 100644 --- a/lib/puppet/resource/type.rb +++ b/lib/puppet/resource/type.rb @@ -138,10 +138,12 @@ class Puppet::Resource::Type end end - # Make an instance of our resource type. This is only possible - # for those classes and nodes that don't have any arguments, and is - # only useful for things like the 'include' function. - def mk_singleton_resource(scope) + # Make an instance of the resource type, and place it in the catalog + # if it isn't in the catalog already. This is only possible for + # 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) type == :definition and raise ArgumentError, "Cannot create resources for defined resource types" resource_type = type == :hostclass ? :class : :node @@ -161,7 +163,7 @@ class Puppet::Resource::Type if parent parent_resource = scope.catalog.resource(resource.type, parent) unless parent_resource - parent_type(scope).mk_singleton_resource(scope) + parent_type(scope).ensure_in_catalog(scope) end end diff --git a/spec/unit/parser/compiler_spec.rb b/spec/unit/parser/compiler_spec.rb index 53b7085..95f3853 100755 --- a/spec/unit/parser/compiler_spec.rb +++ b/spec/unit/parser/compiler_spec.rb @@ -580,7 +580,7 @@ describe Puppet::Parser::Compiler do it "should evaluate each class" do @compiler.catalog.stubs(:tag) - @class.expects(:mk_singleton_resource).with(@scope) + @class.expects(:ensure_in_catalog).with(@scope) @scope.stubs(:class_scope).with(@class) @compiler.evaluate_classes(%w{myclass}, @scope) @@ -591,7 +591,7 @@ describe Puppet::Parser::Compiler do @resource.expects(:evaluate).never - @class.expects(:mk_singleton_resource).returns(@resource) + @class.expects(:ensure_in_catalog).returns(@resource) @scope.stubs(:class_scope).with(@class) @compiler.evaluate_classes(%w{myclass}, @scope) @@ -601,7 +601,7 @@ describe Puppet::Parser::Compiler do @compiler.catalog.stubs(:tag) @resource.expects(:evaluate) - @class.expects(:mk_singleton_resource).returns(@resource) + @class.expects(:ensure_in_catalog).returns(@resource) @scope.stubs(:class_scope).with(@class) @compiler.evaluate_classes(%w{myclass}, @scope, false) @@ -638,7 +638,7 @@ describe Puppet::Parser::Compiler do @scope.stubs(:class_scope).with(@class) Puppet::Parser::Resource.stubs(:new).returns(@resource) - @class.stubs :mk_singleton_resource + @class.stubs :ensure_in_catalog @compiler.evaluate_classes(%w{myclass notfound}, @scope).should == %w{myclass} end end @@ -678,7 +678,7 @@ describe Puppet::Parser::Compiler do @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) node_resource = stub 'node resource', :ref => "Node[c]", :evaluate => nil, :type => "node" - node_class.expects(:mk_singleton_resource).returns(node_resource) + node_class.expects(:ensure_in_catalog).returns(node_resource) @compiler.compile end @@ -688,7 +688,7 @@ describe Puppet::Parser::Compiler do @compiler.known_resource_types.stubs(:node).with("default").returns(node_class) node_resource = stub 'node resource', :ref => "Node[default]", :evaluate => nil, :type => "node" - node_class.expects(:mk_singleton_resource).returns(node_resource) + node_class.expects(:ensure_in_catalog).returns(node_resource) @compiler.compile end @@ -698,7 +698,7 @@ describe Puppet::Parser::Compiler do @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) node_resource = stub 'node resource', :ref => "Node[c]", :type => "node" - node_class.expects(:mk_singleton_resource).returns(node_resource) + node_class.expects(:ensure_in_catalog).returns(node_resource) node_resource.expects(:evaluate) @@ -707,7 +707,7 @@ describe Puppet::Parser::Compiler do it "should set the node's scope as the top scope" do node_resource = stub 'node resource', :ref => "Node[c]", :evaluate => nil, :type => "node" - node_class = stub 'node', :name => "c", :mk_singleton_resource => node_resource + node_class = stub 'node', :name => "c", :ensure_in_catalog => node_resource @compiler.known_resource_types.stubs(:node).with("c").returns(node_class) diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb index e725751..7b240bb 100755 --- a/spec/unit/resource/type_spec.rb +++ b/spec/unit/resource/type_spec.rb @@ -580,29 +580,29 @@ describe Puppet::Resource::Type do end it "should create a resource instance" do - @top.mk_singleton_resource(@scope).should be_instance_of(Puppet::Parser::Resource) + @top.ensure_in_catalog(@scope).should be_instance_of(Puppet::Parser::Resource) end it "should set its resource type to 'class' when it is a hostclass" do - Puppet::Resource::Type.new(:hostclass, "top").mk_singleton_resource(@scope).type.should == "Class" + Puppet::Resource::Type.new(:hostclass, "top").ensure_in_catalog(@scope).type.should == "Class" end it "should set its resource type to 'node' when it is a node" do - Puppet::Resource::Type.new(:node, "top").mk_singleton_resource(@scope).type.should == "Node" + Puppet::Resource::Type.new(:node, "top").ensure_in_catalog(@scope).type.should == "Node" end it "should fail when it is a definition" do - lambda { Puppet::Resource::Type.new(:definition, "top").mk_singleton_resource(@scope) }.should raise_error(ArgumentError) + lambda { Puppet::Resource::Type.new(:definition, "top").ensure_in_catalog(@scope) }.should raise_error(ArgumentError) end it "should add the created resource to the scope's catalog" do - @top.mk_singleton_resource(@scope) + @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.mk_singleton_resource(@scope) + @middle.ensure_in_catalog(@scope) @compiler.catalog.resource(:class, "top").should be_instance_of(Puppet::Parser::Resource) end @@ -610,40 +610,40 @@ describe Puppet::Resource::Type do 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 - lambda { othertop.mk_singleton_resource(@scope) }.should raise_error(Puppet::ParseError) + lambda { othertop.ensure_in_catalog(@scope) }.should raise_error(Puppet::ParseError) end it "should not create a new resource if one already exists" do @compiler.catalog.expects(:resource).with(:class, "top").returns("something") @compiler.catalog.expects(:add_resource).never - @top.mk_singleton_resource(@scope) + @top.ensure_in_catalog(@scope) end it "should return the existing resource when not creating a new one" do @compiler.catalog.expects(:resource).with(:class, "top").returns("something") @compiler.catalog.expects(:add_resource).never - @top.mk_singleton_resource(@scope).should == "something" + @top.ensure_in_catalog(@scope).should == "something" end it "should not create a new parent resource if one already exists and it has a parent class" do - @top.mk_singleton_resource(@scope) + @top.ensure_in_catalog(@scope) top_resource = @compiler.catalog.resource(:class, "top") - @middle.mk_singleton_resource(@scope) + @middle.ensure_in_catalog(@scope) @compiler.catalog.resource(:class, "top").should equal(top_resource) end # #795 - tag before evaluation. it "should tag the catalog with the resource tags when it is evaluated" do - @middle.mk_singleton_resource(@scope) + @middle.ensure_in_catalog(@scope) @compiler.catalog.should be_tagged("middle") end it "should tag the catalog with the parent class tags when it is evaluated" do - @middle.mk_singleton_resource(@scope) + @middle.ensure_in_catalog(@scope) @compiler.catalog.should be_tagged("top") 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.
