On Jul 3, 2012, at 5:04 AM, Brice Figureau wrote:
> On 03/07/12 01:03, Luke Kanies wrote:
>> On Jul 2, 2012, at 1:59 PM, Brice Figureau wrote:
>>
>>> Hi,
>>>
>>> While wrapping up my compiler performance patch (soon to be
>>> released), I stumbled on this test (in compiler_spec.rb):
>>>
>>> it "should fail to add resources that conflict with existing
>>> resources" do path = make_absolute("/foo") file1 =
>>> Puppet::Type.type(:file).new :path => path file2 =
>>> Puppet::Type.type(:file).new :path => path
>>>
>>> @compiler.add_resource(@scope, file1) lambda {
>>> @compiler.add_resource(@scope, file2) }.should
>>> raise_error(Puppet::Resource::Catalog::DuplicateResourceError) end
>>>
>>> I'm wondering what's the point in adding already compiled RAL
>>> resources to a compiler, especially when those are even not
>>> inheriting from Puppet::Resource.
>>>
>>> The reason I'm asking this is that I added two methods to
>>> Puppet::Resource (namely class? and stage?) to prevent the DRY
>>> smell of doing: resource.type.to_s.downcase == "stage" (and
>>> equivalent for "class")
>>>
>>> My intent was to reduce the number of times this code structure is
>>> executed during a compilation (mainly to prevent creation of so
>>> many short-lived String objects).
>>>
>>> Did I miss anything obvious?
>>
>> Probably not. This is a pretty hideous aspect of the system right
>> now, and until we can merge Puppet::Type and Puppet::Resource, it's
>> probably going to be confusing to everyone who isn't me (it's my
>> fault, which is why it's not confusing to me).
>>
>> Currently, Puppet::Type and Puppet::Resource are entirely separate
>> class hierarchies, although I would like the resource-instance
>> behaviors of Puppet::Type to be merged into Puppet::Resource at some
>> point.
>
> That would make sense, but this will really blur the use cases. I'm not
> sure you'd want to do that. That's also something that I always found
> strange: the Catalog can contain 3 types of totally different entities
> (parser resources, then resources, then types instances).
Yeah, although parser resources are inherited from resources. I agree this is
horrible, and I'd love to fix it. They do all have the same basic interface
now, though.
>> In general, and *almost* certainly for the cases of Catalog, they
>> behave identically. I expect that what's happening here is that it
>> doesn't much matter what kind of resource we add, the failure should
>> be the same.
>
> But that's the catalog role to say that no two resources can be added.
> This test really should be in the catalog specs.
I think I added it because I wanted to make sure the error actually propagated
up. That is, it's important that the compiler fail when this exception
happens, regardless of where it's from.
>> It sounds like you think we should be adding
>> Puppet::Resource instances, which is probably right, but maybe this
>> code is older, from before that class existed or something.
>
> Actually I think we should add Puppet::Parser::Resource :)
>
>> This test is important because (I think) Catalog is used for the test
>> at compile time, so you're right it would make more sense to use
>> Puppet::Resource here, rather than Puppet::Type.
>>
>> Does that help?
>
> Well, that's how I fixed the test in the performance-fixes branches, so
> I guess it helped :)
:)
--
Luke Kanies | http://about.me/lak | http://puppetlabs.com/ | +1-615-594-8199
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
