On 03/07/12 22:15, Luke Kanies wrote:
> 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.
Maybe we could separate this interface in its own type (like a
CatalogEntity or something (and I just proved again I was really bad at
naming))?
>>> 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.
Yes, that makes sense.
>>> 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.
