On Thursday, September 5, 2013 4:31:07 PM UTC-5, Jeff McCune wrote:
>
> On Tue, Sep 3, 2013 at 12:44 PM, Luke Kanies
> <lu...@puppetlabs.com<javascript:>
> > 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.
>
Are containment and declaration of a given class really separate concerns?
One class cannot contain another if the other is never declared, so I would
argue that they are *not* separate. Inasmuch as containment is an
order-of-application issue, and Puppet has had 'require' and 'before'
resource metaparameters to manage order of application since the
introduction of parameterized classes, I would furthermore argue that
whether or not declaration and containment *should* be considered separate
in some abstract sense, in practice they never have been separate.
> 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.
>
Do you see specific edge cases or qualifiers that do not already exist?
>
> 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.
>
>
I cannot express how much I would love it if parameterized-style class
declarations worked like that. As far as I'm concerned, the fact that they
don't work that way is the headliner of the example and the root of the
problem. Indeed, as long as we're talking separation of concerns, the fact
that parameterized-style declarations don't currently work that way is
based in a separation of concerns problem itself: parameterized-style
declarations both declare a class AND bind values to its parameters. There
are alternative, uncoupled mechanisms for each of those, so they are
clearly separate.
It's not as clear that containment and inclusion are separate. You can
write a contain() function that does not provide declaration semantics, but
it must fail if the named class is not declared. Moreover, that would make
'contain' tricky to implement such that its use does not create an
evaluation-order dependency -- exactly the same sort of problem that the
example presents, in fact. It could probably be done, but it would require
lazy evaluation, which would have to contend with all the other lazy
evaluation that Puppet performs, some of which can affect whether the
function must succeed or fail.
> 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...
>
It has never been quite that way, and that has always been my greatest
objection to parameterized-style class declarations. They can be used
together with include/require, but only if the one allowed
parameterized-style declaration is evaluated before any of the
non-parameterized ones.
> 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.
>
>
That would be simply lovely. I'd buy the T-shirt. I'll even go file the
bug ticket (using 'include' instead of 'contain') if you're saying it would
be accepted.
If that bug were fixed, would it assuage your concerns about a contain()
function having class declaration semantics?
> 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.
>
This is really nothing new. On one hand, I feel confident in estimating
the number of anchor-pattern analogues to your example in use outside test
suites at roughly -1. On the other hand, users managing the parse order of
their manifests has been par for the course since the introduction of
parameterized classes, wherever parameterized-style class declarations are
used. The problem is there, not inherently in the proposed semantics of
contain().
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.
>
I think you're flipping cause and effect. Any mechanism that adds classes
to the catalog in an exclusive way is inherently a source of the kind of
parse-order problem you are (rightly) concerned about. That giving
contain() declaration semantics would cause problems in conjunction with
such ill-fitting mechanisms is a symptom of that deeper problem, not a
problem in its own right. The same kind of problem would be felt by users
of 'include', and even by users of resource-style class declaration syntax.
>From a user perspective, if contain() is implemented without declaration
semantics then my own usage and my recommendation to everyone will be to
ALWAYS precede contain() with include(). I can live with that, but my
preference would be to avoid the need for it. From a software design
perspective, I don't see a contain() function having class declaration
semantics as commingling of separate of concerns.
John
--
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.
