On Sep 5, 2013, at 2:31 PM, Jeff McCune <j...@puppetlabs.com> wrote:
> On Tue, Sep 3, 2013 at 12:44 PM, Luke Kanies <l...@puppetlabs.com> wrote:
>> +1 to a function expressing containment but not declaring the class (#3).
>> It's the only option of the 4 provided that doesn't have the side-effect of
>> adding the class to the catalog. I've run across situations where a module
>> may want to contain a third party class and relies on some other module to
>> declare the class. "site" modules are a common example.
>
> Sorry, why? What's the benefit of requiring a second call? Or, what's the
> downside of defaulting to 'include' for classes that have not yet been
> evaluated?
>
> The concern of adding a class to the catalog is distinctly separate from the
> concern of containing a class. Smashing the two concerns together makes it
> difficult to address all of the concerns. The benefit of requiring a second
> call is that the concerns are much easier to address directly without edge
> cases and qualifiers for class parameters.
>
> I mentioned a site module. Consider this example. What is the result?
>
> 1 class site::motd($template = 'site/motd') {
> 2 notify { 'motd': message => "template is $template" }
> 3 }
> 4
> 5 class goal {
> 6 notify { "site::motd::template":
> 7 message => "::site::motd::template is ${::site::motd::template}
> (should be ${::node_template})"
> 8 }
> 9 }
> 10
> 11 class container {
> 12 contain('site::motd')
> 13 }
> 14
> 15 node default {
> 16 $node_template = 'site/motd.jeff'
> 17 class { 'container': }
> 18 -> class { 'goal':}
> 19
> 20 class { 'site::motd': template => $node_template }
> 21 }
>
> The result I expect is that the class declaration on line 20 trumps all other
> includes because it is the most specific regarding explicit class parameters.
>
> The behavior we actually get with Puppet right now surprises me because I'm
> quite confident that we decided include was fine to use along with resource
> style class declarations so long as there is only one resource style
> declaration... However, what we actually get is this:
>
> (3.3.0-rc2-16-gcfe704e)
> $ puppet apply -v contain.pp
> Error: Duplicate declaration: Class[Site::Motd] is already declared; cannot
> redeclare at /workspace/serious/src/puppet/contain.pp:20 on node
> agent1.example.org
> Error: Duplicate declaration: Class[Site::Motd] is already declared; cannot
> redeclare at /workspace/serious/src/puppet/contain.pp:20 on node
> agent1.example.org
>
> This is a bug in my opinion. The catalog should compile and apply and we
> should see "::site::motd::template is site/motd.jeff (should be
> site/motd.jeff)" printed out.
The resulting behavior is exactly what I expected, but then… I would. I'm
comfortable saying something like 'include should never evaluate a class that
takes parameters, even if all defaults are available', but isn't that separate
from whether 'contain' should call 'include'? That is, there might be a bug
here, but I don't see how it's related to 'contain'. The perceived bug is
include's fault, not contain's.
> In any event, it's important that we allow one class to contain another class
> and defer explicit class parameters to other places. If contains has the
> side effect of declaring the class then this isn't possible unless the end
> user carefully manages the parse order of their manifests.
Ah; that's a completely different issue:
"'include' should defer evaluation of classes with parameters in case a more
specific evaluation is available"
Right?
>> A separate, focused, function also keeps our support matrix small and more
>> maintainable. We'll want to implement the behavior consistently across all
>> the ways of adding classes to catalogs if we implement the behavior for one
>> of them. These behaviors are burdensome to maintain over time. A
>> contains() function allows the behavior to work with all the ways of adding
>> classes to the catalog while allowing us to maintain the code in one
>> supported manner.
>
> Can you elaborate? I don't know what you mean. What's burdensome about
> having 'contain' call 'include'?
>
> If contain calls include, how can you be sure some other module doesn't
> declare the class before you when you want to provide explicit class
> parameters yourself? You can't be sure unless you take on the burden of
> carefully managing the parse order of the manifests to make absolutely sure
> your resource style declaration happens before include, contains, require, or
> whatever new way we invent to add classes to the catalog in an exclusive way.
See above.
> Do you think it's a mistake for 'require' to call 'include'?
>
> I've never understood why require() exists in the language and I've never
> used the function in any of the manifests or modules I've written.
>
> Yes, it's a mistake for require to call include. If the purpose of require
> is to require another class and it has the side effect of including the class
> in a manner that prevents me from later specifying explicit class parameters,
> then yes it's a mistake to smash these concerns together.
>
> require should require, include should include and contain should contain.
> Each function doing more or doing less than these clear and direct things is
> a mistake.
I couldn't agree less. As Patrick mentioned, if people actually use contain,
you'll see a lot of 'include(foo); contain(foo)'. If I were using it, I'd
literally write something like a 'contain_and_include' function, because this
would annoy me a lot.
As long as the above bug were fixed (assuming others agree it's a bug), there
shouldn't be a problem at all. include should be idempotent; you've just found
an area where it's apparently not.
--
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 unsubscribe from this group and stop receiving emails from it, send an email
to puppet-dev+unsubscr...@googlegroups.com.
To post to this group, send email to puppet-dev@googlegroups.com.
Visit this group at http://groups.google.com/group/puppet-dev.
For more options, visit https://groups.google.com/groups/opt_out.
