-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jeff,
Thanks for the concise write-up, it was really helpful for parsing through all
of the different arguments.
I would like to second the idea that Aaron Grewell had earlier that some sort
of exception handling be allowed and that some other method for doing
the "if I have class A, then do something with class A, otherwise, don't"
scenario is provided. Adding the methods to stdlib would work it just needs
to be available before Telly drops.
I mentioned in a previous thread that I don't see an issue with having multiple
identical resources compiled across the code base and I'd like to add
that to this thread since it's related.
class a {
package { 'foo': ensure => 'present' }
}
class b {
package { 'foo': ensure => 'present' }
}
include 'a'
include 'b'
Should work. However, if the two resources differ, this should be a compile
error. In a perfect world, you wouldn't have this issue, but it shouldn't
be an error since you're applying identical code.
I would also like to make a related request...
Whenever the Puppet core is to change in a way that will break existing code, I
would like a utility to parse my entire code base, whether or not is
is being actively used, to identify potential areas of concern. Right now,
there are deprecation warnings, but those are only output when the code is
run, not if the code is simply in the code base.
puppet-lint helps with some of this and is appreciated but not quite adequate
and the edge cases are difficult to catch.
It feels like Puppet is working its way toward a two pass compile, one for
static code portions and one for dynamic portions. While potentially less
efficient, it would add greater room for the flexibility that people seem to
want overall.
Thanks,
Trevor
On 01/25/2012 03:17 PM, Jeff McCune wrote:
> On Thu, Jan 19, 2012 at 9:18 AM, Nigel Kersten <ni...@puppetlabs.com> wrote:
>> I'm looking for strong opinions on whether we should or shouldn't deprecate
>> the defined() function for Telly, the next major Puppet release this year.
>>
>> jcbollinger put it quite well in another thread:
>>
>> "Use of the "defined" function introduces a parse-order dependency, and the
>> additional work you need to do to ensure that that dependency is always
>> fulfilled overcomes any simplicity advantage that might otherwise exist."
>
> I'm coming into this thread a bit late so I'm going to take the
> strategy of replying to the OP and pasting a lot of the major comments
> about the related issues into this reply and then addressing those
> directly, inline.
>
> The TL;DR version is that I strongly believe defined() is an
> anti-pattern and should be removed from Puppet core.
>
> For those who would still like to have it available I think it should
> be added to the stdlib module as two new functions: already_defined()
> and already_declared(). The already_ prefix is to clearly communicate
> these are parser-order dependent functions and they _do not_ operate
> on the final catalog itself.
>
> Finally, for those who want 100% backwards compatibility, we publish a
> new module in addition to, and compatible with, stdlib that provides
> the existing behavior of defined().
>
>
> And now for the longer version of why I have this strong opinion:
>
> Throughout the replies in this thread, many people are using defined()
> to test of a resource or class has been added to the catalog and if
> not, add a resource with the same type and title to the catalog. I'm
> going to call this the "if not defined then declare" anti-pattern.
> I'll explain why it's an anti-pattern at the conclusion. I hope
> showing the pattern to be an anti-pattern will be sufficient to show
> the pattern should be avoided when publishing modules.
>
> Nick Fagerlund said:
>> Defined() doesn't suck! It's a 100% reliable way to check what classes and
>> defined types are available to the autoloader.
>
> This is an accidental feature and orthogonal to the intent of defined.
> There's also not much evidence that the Puppet community is
> leveraging defined() in this manner. Finally, we should implement a
> first-class and supported function to provide this feature in a clear
> way. is_available() perhaps.
>
> Ashley Penney said:
>> if ! defined(Mysql_user ["${user}@${host}"]) { mysql_user {
>> "${user}@${host}": ... } }
>
> This is the "if not defined then declare" anti-pattern.
>
> Felix Frank said:
>> If your master processes this before the *other* declaration of that
>> mysql_user{}, you're back to square one and get errors about multiple
>> resource declaration.
>
> This is part of why "if not defined then declare" is an ant-pattern.
>
> Nan Liu said:
>> "We should at least differentiate defined vs. declared"
>
> Yes, certainly. We also need to keep in mind that a function can only
> operate on a partial catalog. A function can never operate on a
> complete catalog. Therefore, we need to think of these as "defined
> _yet_" and "declared _yet_" with respect to parse order.
>
> My proposal here is to add already_defined() and already_declared() to
> stdlib. The names are only an example, I don't really care what
> they're called so long as their names take into account the fact that
> a function can only operate on a partially compiled catalog.
>
> Dan Bode said:
>> * static resources in a defined resource type (avoids having to use classes
> to store all static dependencies)
>
> This looks like the anti-pattern, but is not IFF the static resource
> being declared is entirely "private" to the module. If the static
> resource is something more "public" or "common" like a resource for a
> database user/password then this _is_ the anti-pattern.
>
>> * the big reason I keep on leaning on it is for package dependencies.
>
> Using defined() for package dependencies is always an anti-pattern.
> Package dependencies are always "common" and therefore two modules
> that have the same package dependency need to express this as a third
> class. If they don't, then catalogs will have the same resource
> contained in different classes from compilation to compilation because
> of the parse order nature of functions.
>
>> How about deprecating defined?(Type['title']), but allowing it to accept a
>> resource hash?
>
> For the case of package dependencies, moving the dependencies to
> another module / class is much more preferable to this pattern because
> there is a high probability other modules will need the dependencies
> satisfied as well. If the dependencies are not in their own module or
> class then the modules that share common package dependencies must
> repeat this anti-pattern and must know the exact way in which the
> package resource has been declared.
>
> Felix Frank wrote:
>
>> ... endorsing defined() abuse in such a manner will lead to bad (and ugly)
>> code all over the place ...
>
> I share this opinion. Strongly. While the if not defined then
> declare anti-pattern works, and works well for the internal
> organization of a set of modules by a single author it has the
> undesirable side effect of requiring other module authors who depend
> on the same resources to repeat the anti-pattern.
>
> jcbollinger wrote (in response to, "the big reason I keep on leaning
> on it is for package dependencies"):
>
>> You describe one of the core use cases for virtual resources.
>
> Yes, but unfortunately virtual resources are difficult to leverage in
> a module re-use setting because of their global state. I consider
> virtual resources to be a feature for "end users" rather than a tool
> for "module authors" who are publishing to the world.
>
> For example, I expect some users to write Package <| |> { ensure =>
> present } which may wreck havoc with virtual resources I've declared
> for "internal use" by my modules that I'm publishing.
>
> Cody said:
>> Defining (virtual resources for) all somewhat common packages in a central
>> location becomes unrealistic when you no longer "control" the code that is
>> in every module you use.
>
> Yes, this is why I consider virtual resources to be a tool for
> end-users and not module authors.
>
> Aaron Grewell wrote:
>
>> What makes defined() so different from the code that implements require?
>
> What makes it so different is that defined() is used primarily with
> resources, which cannot be declared multiple times, while require() is
> used primarily with classes which can be declared multiple times but
> are only ever added to the catalog once.
>
>> Shouldn't "if not defined" be the same as "if a require would fail"? That
>> seems to be what people are expecting, why not give it to them?
>
> It basically is. It's an anti-pattern because even though that's what
> people expect, the behavior of the system is to change behavior from
> catalog compilation to catalog compilation. People don't expect this
> aspect, hence why I strongly believe it satisfies the, "Some repeated
> pattern of action, process or structure that initially appears to be
> beneficial, but ultimately produces more bad consequences than
> beneficial results" element.
>
> Felix Frank wrote:
>
>> Perhaps there needs to be some kind of "Forge common" module that by policy
>> can only ever declare virtual resources (packages are a prominent example).
>
> This is a good idea, but untenable. We can't require new Puppet users
> to understand this level of complexity and intricacy in order to use
> modules from the Forge.
>
> Ashley Penney wrote:
>
>> ... why does defined create a parse order dependency and why is it not able
>> to search through the entire catalog to see if the class is defined?
>
> defined() is parse order dependent because functions are only ever
> evaluated during the process of _compiling_ a catalog. They're never
> evaluated _after_ a catalog is compiled. Therefore, the idea of the,
> "entire catalog" doesn't really exist when we're talking about parser
> functions because we only have access to, "the catalog we've built up
> (compiled) _so far_."
>
> Hope this helps.
>
> Dan Bode said:
>
>> Until there is a way to distinguish between collection of regular versus
>> virtual resources, I hope that we don't do anything that advocates the usage
>> of virtual resources.
>
> This is my opinion as well. We, as module authors and publishers,
> can't use virtual resources as a tool in our toolbox for this reason.
>
> Nigel Kersten said:
>
>> Please lets focus the conversation here on how things *should* be.
>
> Yep.
>
> We should be able to declare dependencies of a module in a way that is
> deterministic and results in the same resources being added to the
> same catalog given the same set of manifests. We can, today, using
> what I consider the "third party" module pattern. That is, if you
> have a dependency like Java that another module is likely to have then
> Java should be a third party to your module and the other module. We
> can do this today which satisfies the, "A refactored solution exists
> that is clearly documented, proven in actual practice and repeatable."
> element of an Anti-Pattern.
>
> For the original question, we also have a place to move the previous
> functionality to, the stdlib module. Therefore, I think we should
> remove declared() and defined() from Puppet core entirely for Telly
> and add more clear replacements to stdlib. Finally, add
> backwards-compatibility to another module and publish it.
>
> -Jeff
>
- --
Trevor Vaughan
Vice President, Onyx Point, Inc.
email: tvaug...@onyxpoint.com
phone: 410-541-ONYX (6699)
pgp: 0x6C701E94
- -- This account not approved for unencrypted sensitive information --
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
iQEcBAEBAgAGBQJPIKSQAAoJECNCGV1OLcypp6gIAJQNC2qENy1+tGjOCrcyiq10
ZIp5CCxS1d2Jof4bltIxKNhhiNlX1d5YSxM6epFwGiL8ZSqOx0QtE9U04DTOe2iJ
ThQ+AjePFC+8znzeTJHklUq51q2tOGBzsRjv3SyVWpd1hloOnvU1jx16a4R2oWHI
2Z/4zoCNy3bk7e2hKTCKhkBmQmmxA0H1PIW5eCTsFZEYrvp8nvOrjms7KbD5tXHE
G+aSHPYG+bvxks+lKs8pulFv7xBN1EBf+0yoe/y+iLqc6NZvlRzHHGb6VtLrA1HE
f8qPqrodEPptBtyppG9vSLIVHo9vOQP4q9J98JDLGdFdYZAzCXmQut+9WDMMN2s=
=3m7I
-----END PGP SIGNATURE-----
--
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com.
To unsubscribe from this group, send email to
puppet-users+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en.
<<attachment: tvaughan.vcf>>
