RFC: Deprecate defined() function for Telly.
Nigel Kersten
Nigel Kersten
Product Manager, Puppet Labs
R.I.Pienaar
----- Original Message -----
> 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.
First choice would be to make it reliable, that seems unlikely though
+1, I'd also make the scope.classes variable private somehow for similar
reasons.
Walter Heck
only done in badly designed code. Getting rid of it will force people
to use better designs.
cheers,
Walter Heck
> --
> You received this message because you are subscribed to the Google Groups "Puppet Users" group.
> To post to this group, send email to puppet...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-users...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/puppet-users?hl=en.
>
--
Walter Heck
--
follow @walterheck on twitter to see what I'm up to!
--
Check out my new startup: Server Monitoring as a Service @ http://tribily.com
Follow @tribily on Twitter and/or 'Like' our Facebook page at
http://www.facebook.com/tribily
Nick Fagerlund
and defined types are available to the autoloader. I challenge anyone
to find me an example of this usage that fails.
But trying to use it to see whether a specific resource instance has
been declared is pure madness and must be stopped. It's defined(), not
declared(), and it shouldn't accept resource references as arguments
at all.
R.I.Pienaar
----- Original Message -----
> Defined() doesn't suck! It's a 100% reliable way to check what
> classes and defined types are available to the autoloader. I challenge anyone
> to find me an example of this usage that fails.
can you give an example of this use case pls?
Nick Fagerlund
sure if anyone WANTS a reliable way to test the autoloader. (Obviously
people do want a way to check for resource instances, which is why
defined() keeps getting used for that...)
But anyway! Say you make a module for a network service and you want
it to be able to manage its own firewall rule. You know of a defined
type for firewall rules, and you're using it, but you want your module
to be portable and you know of good reasons why someone wouldn't be
using your iptables module.
So, you can conditionally declare the rule if the defined type is
available to the autoloader, and otherwise you don't attempt to manage
the firewall and expect that the user has read the documentation and
will make a hole for the service themselves.
if defined(firewall::iptables::rule) {
firewall::iptables::rule {'mysql_server':
...etc. etc.
}
}
See? It's just a way to peek around at what the user has installed.
Which... maybe implies that it should be renamed to "installed()."
Dunno.
R.I.Pienaar
right, I must have missed this behavior previously, yeah this has value
and should be retained, checking for declared resources should go then
Ashley Penney
Felix Frank
On 01/19/2012 09:22 PM, Ashley Penney wrote:
> An example:
>
> if ! defined(Mysql_user ["${user}@${host}"]) {
> mysql_user { "${user}@${host}":
> password_hash => mysql_password($password),
> require => File["/root/.my.cnf"],
> }
> }
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.
I can see only pain down this road.
Nick's example on the other hand is quite enticing, I think. We want to
keep that (and include it in some Best Practices ;-)
Dan Bode
Dan Bode
Felix Frank
sorry if I come around bluntly about this, but:
On 01/20/2012 10:00 AM, Dan Bode wrote:
> * static resources in a defined resource type (avoids having to use
> classes to store all static dependencies)
>
> * the big reason I keep on leaning on it is for package dependencies.
> Often something needs an additional package installed (and it is
> possible that other modules may have that same package dependency, and I
> don't want to have to create a new class every time that I need another
> package (especially for something complicated that may have tons of
> package dependencies)
I disagree. The coding overhead for a simple wrapper class is not much
larger than adding if defined(); of course, there is the matter of
organizing those wrappers in your modules, though.
Still, endorsing defined() abuse in such a manner will lead to bad (and
ugly) code all over the place (how do you protect against redeclaration
outside your own defined type?)
> puppet-apt has a relevant pull request where someone wants to wrap a
> defined? around python-software-properties for this exact reason
>
> https://github.com/puppetlabs/puppet-apt/pull/10
Again, I find the endorsement of clumsy design questionable. Common
dependencies should be isolated and exported into a mutual dependency
(module).
If there are real life difficulties that make such workflows impossible
at the moment, I still think that it's a bad idea to root flaw-prone
workarounds ever deeper into the module pool.
> How about deprecating defined?(Type['title']), but allowing it to accept
> a resource hash? This would definitely satisfy my use cases while
> alleviating concerns about resource attribute conflicts/parse order
> dependencies
>
> if defined?(
> {
> package['foo'] => { ensure => present }
> }
> ) {
> package { 'foo': ensure => present }
> }
Interesting. This will add some complexity to the DSL, which I'd be wary
of, but if there are truly compelling use cases, this might be
worthwile? I'm not sure. Does this not suffer from the same problems the
current defined() has?
Sincerely,
Felix
Nan Liu
So there is a patch for your specific use case. We should at least
differentiate defined vs. declared and a patch was written to split
off defined() vs. declared():
http://projects.puppetlabs.com/issues/3124
The warning is simply:
Puppet.warning "Detecting puppet resource via defined() function is
deprecated, use declared(). Detecting whether a class or resource type
is defined is still supported."
I think this part of the functionality makes sense to retain. For
those who care, feel free to take a peak at the code:
https://github.com/nanliu/puppet/tree/ticket/2.7.x/3124
Thanks,
Nan
Ashley Penney
jcbollinger
On Jan 19, 2:17 pm, Nick Fagerlund <nick.fagerl...@puppetlabs.com>
wrote:
rather document module and class dependencies, and expect to have
catalog compilation fail if I try to use classes or other module
features whose documented dependencies are not available. That's a
style and best practices position, however. I would be satisfied to
have only defined()'s support for resource references be deprecated.
Alternatively, defined() could be wholly deprecated, but a new
function (e.g. "installed()") introduced that is a work-alike except
for not accepting resource references.
John
jcbollinger
On Jan 20, 3:00 am, Dan Bode <d...@puppetlabs.com> wrote:
> * the big reason I keep on leaning on it is for package dependencies. Often
> something needs an additional package installed (and it is possible that
> other modules may have that same package dependency, and I don't want to
> have to create a new class every time that I need another package
> (especially for something complicated that may have tons of package
> dependencies)
of relying on the defined() function, you can
1) define virtual Package resources in some central place(s) for all
the packages your nodes might want to manage,
2) include that class wherever needed, and
3) *realize* Packages as appropriate wherever you know you need
certain ones.
That avoids parse-order issues, doesn't require you to keep multiple
definitions of the same resource synchronized, and makes it easier to
find specific Package definitions among your manifests. In some cases
you might even be able to leverage collections with selection
predicates to simplify and clarify your manifests. I don't see a
single reason to prefer use of 'defined' for this case.
John
jcbollinger
On Jan 20, 3:38 am, Nan Liu <n...@puppetlabs.com> wrote:
> So there is a patch for your specific use case. We should at least
> differentiate defined vs. declared and a patch was written to split
> off defined() vs. declared():
>
> http://projects.puppetlabs.com/issues/3124
>
> The warning is simply:
>
> Puppet.warning "Detecting puppet resource via defined() function is
> deprecated, use declared(). Detecting whether a class or resource type
> is defined is still supported."
>
> I think this part of the functionality makes sense to retain. For
> those who care, feel free to take a peak at the code:https://github.com/nanliu/puppet/tree/ticket/2.7.x/3124
it being removed from the warning message emitted by defined().
John
Cody
unrealistic when you no longer "control" the code that is in every
module you use. If you obtain five modules from the forge and they
all require a specific package and so all define that package your not
going to convince, nor is it a good design to require everyone to move
the package definitions from that collection of modules. They need to
function as a collection out of the box.
Aaron Grewell
> Defining all somewhat common packages in a central location becomes
> unrealistic when you no longer "control" the code that is in every
> module you use. If you obtain five modules from the forge and they
> all require a specific package and so all define that package your not
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
>
Are we sure it can't be fixed? What makes defined() so different from
the code that implements require? 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?
Aaron Grewell
>
> Are we sure it can't be fixed? What makes defined() so different from
> the code that implements require? 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?
Never mind that last bit, it took me a second to realize the order of
operations would make that Very Hard. A better question would be "do
we have a proper replacement in the pipeline for Telly?" Perhaps
exception handling for require? If not then something's better than
nothing IMHO.
Felix Frank
On 01/20/2012 11:34 PM, Cody wrote:
> Defining all somewhat common packages in a central location becomes
> unrealistic when you no longer "control" the code that is in every
> module you use. If you obtain five modules from the forge and they
> all require a specific package and so all define that package your not
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
Agreed. How can this be accomplished?
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).
A user who wishes to retain the capability of using modules from the
Forge would be required to install this common module, and replace their
own resource declarations with realizations of the common resources.
For this to work, it's definitely a plus that you can override
attributes in collections:
Package<| title == "apache2": |> { ensure => "2.2.12" }
...although that does bear some caveats. Does this still work in recent
versions?
If we can take this for granted, all Forge modules can adhere to that
same standard.
Yes, it's quite a hassle.
No, I didn't think this through very thoroughly ;-)
Just another pair of cents.
Cheers,
Felix
Nigel Kersten
Hi,
Agreed. How can this be accomplished?
On 01/20/2012 11:34 PM, Cody wrote:
> Defining all somewhat common packages in a central location becomes
> unrealistic when you no longer "control" the code that is in every
> module you use. If you obtain five modules from the forge and they
> all require a specific package and so all define that package your not
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
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).
A user who wishes to retain the capability of using modules from the
Forge would be required to install this common module, and replace their
own resource declarations with realizations of the common resources.
For this to work, it's definitely a plus that you can override
attributes in collections:
Package<| title == "apache2": |> { ensure => "2.2.12" }
...although that does bear some caveats. Does this still work in recent
versions?
If we can take this for granted, all Forge modules can adhere to that
same standard.
Yes, it's quite a hassle.
No, I didn't think this through very thoroughly ;-)
Just another pair of cents.
Cheers,
Felix
--
You received this message because you are subscribed to the Google Groups "Puppet Users" group.
To post to this group, send email to puppet...@googlegroups.com.
To unsubscribe from this group, send email to puppet-users...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-users?hl=en.
Ashley Penney
Felix Frank
there was a discussion in the "can we deprecate defined() in Telly"
thread about how we can even begin to design Forge modules without it.
A recurring problem is that multiple modules rely on certain packages,
and there is no good model (yet) to unite their resource declarations.
Therefore it's a common (although imho disgusting) workaround to do
things like
if !defined(Package[foo]) { package { "foo": ensure => installed } }
On 01/20/2012 11:34 PM, Cody wrote:
> Defining all somewhat common packages in a central location becomes
> unrealistic when you no longer "control" the code that is in every
> module you use. If you obtain five modules from the forge and they
> all require a specific package and so all define that package your not
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
Agreed. How can this be accomplished?
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).
A user who wishes to retain the capability of using modules from the
Forge would be required to install this common module, and replace their
own resource declarations with realizations of the common resources.
For this to work, it's definitely a plus that you can override
attributes in collections:
Package<| title == "apache2": |> { ensure => "2.2.12" }
...although that does bear some caveats. Does this still work in recent
versions?
If we can take this for granted, all Forge modules can adhere to that
same standard.
This is a rough sketch of how things might possibly work, and surely has
lots of wrinkles of its own. Still, I'm quite sure we need a proper way
to rid ourselves of the horror that is the parse order dependent check
for defined resources ;-)
Cheers,
Felix
Felix Frank
> Felix, could you take this to a new thread please? I'd really like to
> keep this one focused on the topic at hand if possible :)
I'd had loved to, but I'm apparently too dumb to use E-Mail. Let's try
again ;(
Felix Frank
Matthias Saou
Felix Frank <felix...@alumni.tu-berlin.de> 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 only takes care of the Forge case. The problem of having the same
package being required in multiple places isn't limited to Forge, it's
one I run into locally all the time (and I'm sure I'm not the only one).
On top of that, once you try to cover multiple distributions where
package names diverge, it starts getting hard. And once the packaging
itself is different, as in (non-existing) sub-packages... there is no
end (1).
I don't have the slightest idea as to how all this could be solved in
a clean way, but "requiring a common bit of high level code" isn't the
global solution I'm myself hoping for.
Matthias
(1) Example where a nagios plugin requires the package for the nagios
perl bindings. It's "nagios-perl" for most RPMs, but not split out and
in the main "nagios-plugins" on Gentoo... then for any other plugin
from nagios-plugins RPMs, such as "nagios-plugins-file_age", same
thing. So you get a big mess with duplicate definitions for the main
Gentoo "nagios-plugins" package, or you need yet another layer of high
level code hack :-/
Tim Sharpe
Felix Frank
> The only "clean" way to handle this that I can think of off the top of
> my head
> is for Puppet to start silently discarding duplicate definitions and
> just using
> the first one it comes across (with perhaps a message being logged at info
> level so that it's not completely invisible).
>
> This is far from ideal but so are the other proposed solutions so far :)
No, it really adheres to KISS very nicely.
This reminds me of the discussion of how resource defaults should stack
when they combine from different scopes. (How did that turn out, anyway?)
The vision here would probably be that the compiler collects all
definitions or the same resource, and only dies if any attributes differ.
Sadly, *due* to resource defaults, this could be more likely than one
would think.
Also, depending on how the compiler is implemented, it may be Very Hard
to do.
Cheers,
Felix
Walter Heck
of requirements or an API of some sort? I don't know how this would
work in practice since it just popped in my mind, but I imagine for
instance a monitoring module would expose some kind of hook that other
modules can check for, and if it is found then execute the code
pertaining to monitoring. It would mean a certain abstraction, so
instead of saying an external module needs to implement nagios-perl,
it would just ask for an external module to implement monitoring, and
pass certain parameters to it in order to get that monitoring
implemented. The nagios module would then be responsible for the
actual monitoring stuff.
Come to think of it, this wouldn't take care of the duplicate resource
stuff discussed here. Nevertheless I wanted to put out this thought
anyway as it seems to be a larger issue related to this.
As for the duplicate resource issue, what about a diff/merge strategy:
If duplicate resources are found, tehre are three options:
1. if the two resource were compared and found the same it would just
be allowed and logged to the user
2. if not found the same, merged and logged:
file{x: ensure => present} and
file {x: owner => root}
becomes
file {x: ensure => present, owner => root},
3. if merge conflicts are found (ensure => present and ensure =>
absent in the two definitions) we get an error.
Could that work?
Walter
--
Walter Heck
--
follow @walterheck on twitter to see what I'm up to!
--
Check out my new startup: Server Monitoring as a Service @ http://tribily.com
Follow @tribily on Twitter and/or 'Like' our Facebook page at
http://www.facebook.com/tribily
Aaron Grewell
package { "foo": ensure => installed, exceptDefined => skip}
Or something of that nature. This could also be used in other
situations where you want to bypass default behaviors. We've seen
situations where users want to apply a file if it exists in the module
but otherwise proceed without errors. This could be done like so:
file {"$foo": ensure => present, source => "${foo}.txt", exceptAbsent => skip}
Nigel Kersten
What if there was some standardised way modules expose a sort of list
of requirements or an API of some sort?
Nan Liu
>
>
> On Tue, Jan 24, 2012 at 4:42 AM, Walter Heck <walte...@gmail.com> wrote:
>>
>> What if there was some standardised way modules expose a sort of list
>> of requirements or an API of some sort?
>
>
> I had this thought the other day, but more focused around the higher level
> problem of dependency specification and consumption than Packages in
> particular.
>
> What if you made two functions:
>
> 1. declare_capability("foo")
> This would essentially just create an empty resource, Capability[foo]. If
> multiple modules tried to declare the same capability, it would error, just
> like we do today with duplicate resources.
>
> 2. require_capability("foo")
> This would just declare a requirement relationship to Capability[foo]. If
> it couldn't find that resource, compilation would fail.
I'm actually also interested in detect capability too. For example if
class firewall is included, add the following iptable rules, otherwise
don't include firewall rules in the catalog. It can be implemented as
parameter in the application class, but feels much nicer if I can
simply include firewall, and all the app added firewall rules, vs.
class { 'app1': firewall => true }... I'm aware this might be order
dependent and tricky to implement.
Thanks,
Nan
Walter Heck
> problem of dependency specification and consumption than Packages in
> particular.
implement properly. I'd welcome a proper way to do this.
> What if you made two functions:
>
> 1. declare_capability("foo")
> This would essentially just create an empty resource, Capability[foo]. If
> multiple modules tried to declare the same capability, it would error, just
> like we do today with duplicate resources.
>
> 2. require_capability("foo")
> This would just declare a requirement relationship to Capability[foo]. If
> it couldn't find that resource, compilation would fail.
>
>
> Wouldn't something like this solve the general problem? We don't really want
> to just speak about packages, you generally want to be able to express
> requirements like:
>
> * This module requires a working SSH server
> * This module requires a MySQL database
>
> etc etc
>
> I've deliberately focused this on doing this with functions as the extension
> point as I think it keeps us focused, even if we decide that in the future
> this should be solved in the metadata layer of a module.
Hmm, while I can see your point of using functions, I think a more
"puppety" way of doing this would be to have
require Capability['yadda']
vs
capability{'yadda':
ensure => present
}
We'd also need a way to test if a capability is being provided by the
system as a whole. Ie, a mysql module needs to be able to check if it
needs to include resources for a firewall, monitoring, etc.
cheers,
Nigel Kersten
We'd also need a way to test if a capability is being provided by the
system as a whole. Ie, a mysql module needs to be able to check if it
needs to include resources for a firewall, monitoring, etc.
Walter Heck
> Yeah. I went for functions to try and hide the fact that this is a resource
> underneath, but perhaps modeling this more transparently makes sense.
>
>> We'd also need a way to test if a capability is being provided by the
>> system as a whole. Ie, a mysql module needs to be able to check if it
>> needs to include resources for a firewall, monitoring, etc.
> Something like defined()? ;-)
Come on, I thought we all agreed that defined is Nasty and Evil. At
least make it defined_new() or maybe even defined2() :P
Felix Frank
>>> We'd also need a way to test if a capability is being provided by the
>>> >> system as a whole. Ie, a mysql module needs to be able to check if it
>>> >> needs to include resources for a firewall, monitoring, etc.
>> > Something like defined()? ;-)
> Come on, I thought we all agreed that defined is Nasty and Evil. At
> least make it defined_new() or maybe even defined2() :P
Great, now we're regressing back to thread that Nigel kicked me out of
for this :P
Fine, I'll play.
On 01/19/2012 07:56 PM, Nick Fagerlund wrote:
> Defined() doesn't suck! It's a 100% reliable way to check what classes
> and defined types are available to the autoloader. I challenge anyone
> to find me an example of this usage that fails.
>
> But trying to use it to see whether a specific resource instance has
> been declared is pure madness and must be stopped. It's defined(), not
> declared(), and it shouldn't accept resource references as arguments
> at all.
So there. In that thread, Nick gave pretty much the very use case
example Nan has referred to earlier in this thread, so I hope we can
agree that this can be handled by the (newly introduced?) declared()
function (or stripped down version of defined() that won't allow
resource reference parameters).
On 01/19/2012 09:17 PM, Nick Fagerlund wrote:
> So, you can conditionally declare the rule if the defined type is
> available to the autoloader, and otherwise you don't attempt to manage
> the firewall and expect that the user has read the documentation and
> will make a hole for the service themselves.
>
> if defined(firewall::iptables::rule) {
> firewall::iptables::rule {'mysql_server':
> ...etc. etc.
> }
> }
>
> See? It's just a way to peek around at what the user has installed.
Cheers,
Felix
Nick
> What if you made two functions:
>
> 1. declare_capability("foo")
> This would essentially just create an empty resource, Capability[foo]. If
> multiple modules tried to declare the same capability, it would error, just like
> we do today with duplicate resources.
>
> 2. require_capability("foo")
> This would just declare a requirement relationship to Capability[foo]. If it
> couldn't find that resource, compilation would fail.
>
>
> Wouldn't something like this solve the general problem? We don't really want to
> just speak about packages, you generally want to be able to express requirements
> like:
>
> * This module requires a working SSH server
> * This module requires a MySQL database
More generally, I think it'd be nice to have a way to be able to define
resources, in a way approximately like Puppet already does, but without also
having to say "and if anyone else tries to define the same name resource in any
way, fail".
i.e. Say there were two unrelated modules which said slightly different but
*compatible* things:
Module a:
file { "/foo/bar":
ensure => 'present',
owner => 'root',
content => "blah blah",
}
Module b:
file { "/foo/bar":
ensure => 'present',
mode => '0774',
}
Currently Puppet doesn't allow them to co-exist. It would be nice if instead it
could be told to check these definitions are consistent, and then enforce the
union of the two. The same principle could apply to users, groups, packages,
and presumably any other resources.
See my related point in the thread "constraint checking".
N
Nan Liu
How would this be implemented in a sane way to deal with any
attributes that are hash/array? Merge, merge+unique, fail? What if we
add relationship (require/before) or other meta-parameters to the mix?
If I use the puppet config_version feature to track what resource is
changed by which line of puppet code for auditing purpose, how would I
audit a single attribute that can be due to multiple line of code?
Once I started thinking about define types (which behave like a
resource), it's gets rather complex especially with conditional
branching in the define type.
Don't get me wrong, this clearly would be a useful feature, but I'm
interested only if the rules of how this would behave can be clearly
expressed and understood, otherwise this will be a maze of pain trying
to figure out what part of the code broke something.
Nan
Dan Bode
Hi,
there was a discussion in the "can we deprecate defined() in Telly"
thread about how we can even begin to design Forge modules without it.
A recurring problem is that multiple modules rely on certain packages,
and there is no good model (yet) to unite their resource declarations.
Therefore it's a common (although imho disgusting) workaround to do
things like
if !defined(Package[foo]) { package { "foo": ensure => installed } }
On 01/20/2012 11:34 PM, Cody wrote:
> Defining all somewhat common packages in a central location becomes
> unrealistic when you no longer "control" the code that is in every
> module you use. If you obtain five modules from the forge and they
> all require a specific package and so all define that package your not
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
Agreed. How can this be accomplished?
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).
A user who wishes to retain the capability of using modules from the
Forge would be required to install this common module, and replace their
own resource declarations with realizations of the common resources.
For this to work, it's definitely a plus that you can override
attributes in collections:
Package<| title == "apache2": |> { ensure => "2.2.12" }
...although that does bear some caveats. Does this still work in recent
versions?
If we can take this for granted, all Forge modules can adhere to that
same standard.
This is a rough sketch of how things might possibly work, and surely has
lots of wrinkles of its own. Still, I'm quite sure we need a proper way
to rid ourselves of the horror that is the parse order dependent check
for defined resources ;-)
Cheers,
Felix
Ashley Penney
Nigel Kersten
I don't have a solution but I do want to chime in and state that modules thatexist just to collect together virtual resources is a horrible thing. We're justtalking about working around the way Puppet works right now rather thandefining what the end solution should be.
Felix Frank
> After all, if they've been included on the host all of those attributes
> should apply so I can't see a dangerous downside to this.
Nan provided some killer arguments (imho) in the proper cross-dependency
thread.
Matthias Saou
Nan Liu <n...@puppetlabs.com> wrote:
[...]
Getting into this level of detail is interesting, but quite beyond the
initial simple use case of the package type. As it has already been
pointed out in this thread, the initial problem would be solved by
simply allowing duplicates as long as all parameters are identical,
since it's typically just "package { 'foo': ensure => installed }".
That would also have a whole bunch of new implications, as I can imagine
people changing a single parameter in one place and getting confused as
to why the now get duplicate definition errors, but it would be a heck
of a lot simpler than trying to "merge" definitions together :-)
Matthias
Jeff McCune
> 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
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
- --
Trevor Vaughan
Vice President, Onyx Point, Inc.
email: tvau...@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-----
Ashley Penney
Nigel Kersten
All I know is that telling users "If you download 5 modules from puppet forgemake sure you go through them all, extract any duplicating resources intorandom modules that exist purely to allow you to realize packages" instead leadsto a really bad user experience.
Felix Frank
On 01/26/2012 02:25 AM, Ashley Penney wrote:
> This is a fantastic reply and I appreciate the work you put into it. I
> have just one
> question. As it stands functions can only apply to partial catalogs and
> not to the
> full catalog. Is this a fundamental design decision that cannot be
> changed? Perhaps
> it would be interesting to speculate on what could be done if you had
> the ability to
> use the entire catalog when fully parsed.
This goes in a similar direction as Trevor's comment:
On 01/26/2012 01:55 AM, Trevor Vaughan wrote:
> 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.
Please let's NOT go down that road. You won't get away with two passes.
Think about it: After every pass, some if { } could switch values
depending on what defined() or some other function now returns.
The compiler would need to recursively repeat all its work (don't even
get me started on infinite loops).
That's another road to pain right there IMO.
> I still dislike the third module refactoring. I think it removes a lot
> of power of self-
> contained modules and makes things significantly uglier and more
> difficult when
> combining modules from multiple sources. I wish it could be solved in a
> better
> way within Puppet and I believe it could be with (perhaps optional)
> merging of
> identical resources.
Modules that work in and of itself are desirable, seeing as they're very
elegant. On the other hand, in the worst case you duplicate lots of code
(yes, package { "java": } is not a lot of code), which modules should
normally keep you from doing.
> All I know is that telling users "If you download 5 modules from puppet
> forge
> make sure you go through them all, extract any duplicating resources into
> random modules that exist purely to allow you to realize packages"
> instead leads
> to a really bad user experience.
Uhm, what? o_O
That's not at all what I had in mind. This is a job for *authors*, not
end users.
Thinking about other examples of similar systems (CPAN, Gems, Pear, you
name it), module dependencies are commonplace (and usually coupled with
a system that will automatically resolve them for the end user).
I stronly believe that the Forge is in need of such a system (I believe
Nigel brought up the proposal of metadata for this), and it should be
best practice to design modules to rely on this as much as possible.
BTW Nigel: Separating this thread from the "Cross-module (package)
dependencies" hasn't really worked out, has it? ;)
Sincerely,
Felix
Nick
> 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 think this is a small improvement, but it is still forcing strong coupling -
two parts of the manifest have to know about each other and agree on how things
are defined. It means writing self-contained manifests will still be hard or
impossible, unless *everyone* adopts the (arbitrary) convention of defining only
"ensure => present" or somesuch.
For example, suppose in one place I need a file to exist, and in another I also
need it to be executable. Oh dear, I can't do that.
N
Nick
> All I know is that telling users "If you download 5 modules from puppet forge
> make sure you go through them all, extract any duplicating resources into
> random modules that exist purely to allow you to realize packages" instead leads
> to a really bad user experience.
+1 from me.
Felix Frank
> For example, suppose in one place I need a file to exist, and in another I also
> need it to be executable. Oh dear, I can't do that.
That, and you'd need to merge require/before etc. Such things aren't
trivial. Nan put it this way:
jcbollinger
On Jan 20, 4:34 pm, Cody <c.a.herri...@gmail.com> wrote:
> On Jan 20, 6:39 am, jcbollinger <John.Bollin...@stJude.org> wrote:
>
>
>
> > On Jan 20, 3:00 am, Dan Bode <d...@puppetlabs.com> wrote:
>
> > > * the big reason I keep on leaning on it is for package dependencies. Often
> > > something needs an additional package installed (and it is possible that
> > > other modules may have that same package dependency, and I don't want to
> > > have to create a new class every time that I need another package
> > > (especially for something complicated that may have tons of package
> > > dependencies)
>
> > You describe one of the core use cases for virtual resources. Instead
> > of relying on the defined() function, you can
> > 1) define virtual Package resources in some central place(s) for all
> > the packages your nodes might want to manage,
> > 2) include that class wherever needed, and
> > 3) *realize* Packages as appropriate wherever you know you need
> > certain ones.
>
> > That avoids parse-order issues, doesn't require you to keep multiple
> > definitions of the same resource synchronized, and makes it easier to
> > find specific Package definitions among your manifests. In some cases
> > you might even be able to leverage collections with selection
> > predicates to simplify and clarify your manifests. I don't see a
> > single reason to prefer use of 'defined' for this case.
>
> Defining all somewhat common packages in a central location becomes
> going to convince, nor is it a good design to require everyone to move
> the package definitions from that collection of modules. They need to
> function as a collection out of the box.
interoperable, and module authors indeed want their modules to be
interoperable, then I think it would be relatively easy to persuade
most authors to do it. I can think of ways to make it work, and I'll
write more on that in some other thread.
Sticking to the subject at hand, however, there remains the issue that
defined() does not adequately solve the shared resource problem
anyway. If two modules both rely on the same resource then it is not
safe for one to assume that the definition of that resource provided
by the other meets all its needs. The 'defined' function can maybe
ensure that the manifests compile, but it cannot ensure that the
resulting configuration is correct. That makes it worse than useless
for the purpose, as far as I'm concerned.
John
Felix Frank
> The 'defined' function can maybe
> ensure that the manifests compile, but it cannot ensure that the
> resulting configuration is correct. That makes it worse than useless
> for the purpose, as far as I'm concerned.
Exceptionally good point.
Thomas Bellman
Hash: SHA1
(I know I'm late in responding; I'm a couple of months behind in reading
the mailing-list, and still have 2000 unread. I have read this entire
thread, though.)
On 2012-01-19 18:18, Nigel Kersten 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."
- -1 until there is some other, usable, way of realizing the good things
you can achieve with defined().
My use case for defined() is in definitions that wants to make sure
a package it needs is installed, and where that package varies between
instances of the definitions. For example, I have a definition named
nrpe::check, which configures a Nagios NRPE check. It wants to make
sure the appropriate nagios-plugins-PLUGIN is installed, so it does
(simplified, and just retaining the code dealing with the package):
define nrpe::check($plugin='', $checker='', $args=[], $prefixcmd=[])
{
$pluginpkg = $plugin ? {
'' => "nagios-plugins-${name}",
0 => '',
default => $plugin,
}
if $pluginpkg != '' and ! defined(Package[$pluginpkg]) {
package {
$pluginpkg: ensure => installed,
}
}
}
A user would do things like:
nrpe::check { 'load': args => [ '-w20,10,8', '-c30,18,10' ]; }
and the "nagios-plugins-load" package will automatically be installed.
This becomes interresting when you have:
nrpe::check { 'test_foo': args => ['foo'], plugin => 'nagios-plugins-ourtest'; }
and
nrpe::check { 'test_bar': args => ['bar'], plugin => 'nagios-plugins-ourtest'; }
both using the same plugin, but with different parameters, in different
parts of the manifest for the node. The cincher here is that there are
several dozens of Nagios plugin packages available (EPEL has 63, many
organizations probably have several of their own, and so on).
It would be possible to instead have the nrpe::check definition do
include "nagios_plugin_package::${plugin}"
and force the user to define a class for each plugin they use (and I
could of course "pre-seed" by writing 63 of those myself in my nrpe
module). But it becomes much easier on the users of the nrpe module
if they don't need to do so.
This use of defined() is safe, since there is really only a single
declaration of each Package['nagios-plugins-PLUGIN'] resource.
I'm not fond of defined(), as it is easy to get wrong, but it is also
quite useful in some cases. It would be a shame to deprecate a useful
feature before there is a workable way to achieve the same end results
in place.
/Bellman
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk90nj8ACgkQDGpP8Cv3aqL2WQCeMkEru8FitrIIzuG3HNdQGiPe
18oAnR94buhofKlbMBa/S+6zEtxX5jYa
=iTQw
-----END PGP SIGNATURE-----
Thomas Bellman
Hash: SHA1
(Coming late to the discussion, I know. Sorry.)
On 2012-01-26 11:19, Felix Frank wrote:
[Regarding common modules for installing packages.]
> Thinking about other examples of similar systems (CPAN, Gems, Pear, you
> name it), module dependencies are commonplace (and usually coupled with
> a system that will automatically resolve them for the end user).
A big difference is that in those other systems, you don't typically
see modules on the level of
def add_17(n)
return n + 17
end
And of course, we need another module for doing
def add_23(n)
return n + 23
end
Puppet modules for making sure a single package is installed is
on the same sofistication level as the above two. It's silly.
What would the reaction of the Python, Perl and Ruby communities
be if someone wanted modules like that in PyPI, CPAN and RubyGems?
Why should we want such sillinesses in the Puppet Forge?
/Bellman
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk90ocUACgkQDGpP8Cv3aqK7mgCdEQlPE70a1bJncrmeC2fvqv+V
9OoAn36vzrLoyCsB976MUCmOjeJo/IN5
=tbQM
-----END PGP SIGNATURE-----
jcbollinger
and maintainability. It will not do what you want if the declaration
of the desired package has not yet been parsed, and parse order is
tricky to predict except by means that offer better solutions anyway.
That particular example appears to be a good use case for virtual
resources:
class nrpe::plugins {
@package { 'nagios-plugins-plugin1': ensure => 'installed' }
@package { 'plugin2': ensure => 'installed' }
define nrpe::check($plugin='', $checker='', $args=[], $prefixcmd=[])
{
$pluginpkg = $plugin ? {
'' => "nagios-plugins-${name}",
0 => '',
default => $plugin,
}
realize Package[$pluginpkg]
}
> A user would do things like:
>
> nrpe::check { 'load': args => [ '-w20,10,8', '-c30,18,10' ]; }
>
> and the "nagios-plugins-load" package will automatically be installed.
> This becomes interresting when you have:
>
> nrpe::check { 'test_foo': args => ['foo'], plugin => 'nagios-plugins-ourtest'; }
>
> and
>
> nrpe::check { 'test_bar': args => ['bar'], plugin => 'nagios-plugins-ourtest'; }
>
> both using the same plugin, but with different parameters, in different
> parts of the manifest for the node. The cincher here is that there are
> several dozens of Nagios plugin packages available (EPEL has 63, many
> organizations probably have several of their own, and so on).
>
> It would be possible to instead have the nrpe::check definition do
>
> include "nagios_plugin_package::${plugin}"
>
> and force the user to define a class for each plugin they use (and I
> could of course "pre-seed" by writing 63 of those myself in my nrpe
> module). But it becomes much easier on the users of the nrpe module
> if they don't need to do so.
another avenue to the desired result.
> This use of defined() is safe, since there is really only a single
> declaration of each Package['nagios-plugins-PLUGIN'] resource.
wouldn't call it "safe". It risks breakage if any of the packages in
question indeed are declared elsewhere in your manifests, and it is
insufficient if ever you have a need for any of the plugin packages to
be declared with nonstandard properties (e.g. a specific version).
That usage of defined() requires faithful adherence to usage patterns
that Puppet has no means to check or validate, and there are viable
alternatives (even if you don't like them as well for your specific
case). That's why that use of defined() is under consideration for
deprecation.
> I'm not fond of defined(), as it is easy to get wrong, but it is also
> quite useful in some cases. It would be a shame to deprecate a useful
> feature before there is a workable way to achieve the same end results
> in place.
interoperability problem. I don't expect any fundamental advances in
that area in Telly, but I have high hopes for the following major
release.
John