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
--
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.
