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).
> 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.
> 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 :)
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--
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.
