+1 On Mon, Oct 18, 2010 at 1:29 PM, Paul Berry <[email protected]> wrote:
> 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- 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.
