On Jul 3, 2012, at 1:42 PM, Brice Figureau wrote:
> 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))?
Well, ideally it would just be Puppet::Resource. I don't see the need for a
special Catalog entry, if we can reduce the number of resource classes.
>>>> 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.
>
--
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 [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.
