On Sep 5, 2013, at 2:31 PM, Jeff McCune <[email protected]> wrote: > On Tue, Sep 3, 2013 at 12:44 PM, Luke Kanies <[email protected]> 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 [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/puppet-dev. For more options, visit https://groups.google.com/groups/opt_out.
