(#13610) Add function_available to stdlib

[eshamow]

This function provides a simple wrapper around
Puppet::Parser::Functions.function for access within Puppet manifests.
This will allow users to check whether or not a plugin or functionality
such as hiera is installed on the server.

[kwilczynski]

Hi,

This is definitely a nice idea, but I'd rather see real boolean being returned in both cases, therefore a little update:

function = Puppet::Parser::Functions.function(:notice)
function.is_a?(String) and not function.empty?

Above would yield boolean true, whereas:

function = Puppet::Parser::Functions.function(:abc)
function.is_a?(String) and not function.empty?

Results in false.

This makes this function a little bit more deterministic in terms of its behaviour :)

KW

[eshamow]

Krzysztof,

The idea here is to expose some Puppet internals that otherwise would need a (light) Ruby wrapper. If you don't you need an additional function to grab this info.

This works well enough on the Ruby side...any significant reason to make it a straight-up boolean? It still seems easy enough to test against in its present form.

-Eric

[kwilczynski]

Hi,

I am not quite sure if I understand your reasoning. Is this intended to use for people within Puppet DSL?

KW

[eshamow]

It is. But the effect is the same.

If you say

if function_available('foo') {
do X
} else {
do Y
}

Then with the function as provided, should function_available return false, Y will happen. On any other return value, X will.

So I guess the question is, what's the advantage in obscuring the output/returning LESS information?

[kwilczynski]

Hi,

I still do not follow your reasoning, but after reading description which you have added, I would expect this to work:

matti@acrux ~/test $ RUBYLIB=. cat - | puppet apply 
case function_available('notice') {
 true: { notice 'yes' }
 false: { notice 'no' }
}
notice: Finished catalog run in 0.02 seconds

Like this piece of code:

matti@acrux ~/test $ RUBYLIB=. cat - | puppet apply 
case function_available('notice1') {
 true: { notice 'yes' }
 false: { notice 'no' }
}
notice: Scope(Class[main]): no
notice: Finished catalog run in 0.03 seconds

Just "going by" the sheer fact that :undef, '' (empty string) and false evaluates to boolean false (or FalseClass in Ruby) is still not enough to justify the reason for retuning either string or boolean. So, please either fix it and/or update your description so you wont confuse users :)

Also, I do not want this to change into a bike shedding problem, therefore I leave people from Puppet Labs to decide what behaviour they would prefer :)

KW

[eshamow]

OK I definitely can see that - the description is definitely misleading. Fixing.....

...and fixed. Description now better reflects the actual behavior.

-Eric

[nigelkersten]

I would expect a "function_available" method to return a boolean.

[eshamow]

OK. Modified to return a boolean, spec test updated as well.

[kwilczynski]

Hi Nigel,

[...]

I would expect a "function_available" method to return a boolean.

I felt a little bit like a douche, therefore thank you for making the
final decision. I really appreciate that.

P.S. I care a great deal about stdlib :)

KW

[nigelkersten]

You should totally not feel like a douche :) Many eyes make code better :)

[eshamow]

@kwilczynski +1 to Nigel's comment. That's what pull requests are for :)

[kwilczynski]

@eshamow +1 for implementing and sharing! Open Source is so amazing ;-)

[eshamow]

@kwilczynski It's less awesome than it seems, I'm a PL employee :D I just don't contribute code all that often.

[kwilczynski]

@eshamow haha, lucky you ;-) For being PL employee ;-)

[eshamow]

@kwilczynski Apply! Puppet is always hiring and it's an awesome place to hang your hat.

@nigelkersten This look mergeable to you? (ignore recruiting efforts above :D )

[kwilczynski]

@eshamow you guys don't hire in London :-( Plus, I am not entirely sure if @nigelkersten would be happy to have me on-board (?) :)

[kbarber]

@eshamow reviewing this now.

[kbarber]

This could also be a string comparison:

function == "function_" + arguments[0]

[kwilczynski]

@kbarber Could be, indeed.

[kwilczynski]

@eshamow and @kbarber there is one little bit that is missing, at least I believe so. I mean, the missing bit is error handling on argument type-checking. For instance, calling to_sym on [] and {}, as well as any Float will result in NoMethodError, then for both positive and negative Integer it would return simply NilClass, and finally for empty string '' we'd get ArgumentError being risen.

[kbarber]

Aaah sorry - @kwilczynski is correct, but not as bad as he outlines. The problem is things like this:

Puppet::Parser::Functions.function("".to_sym)
ArgumentError: interning empty string
from (irb):3:in `to_sym'
from (irb):3

Since Puppet treats numbers likes strings, this is pretty much the main issue. Regardless, lets validate the input Eric - adding tests to make sure its a string, not empty etc. Thanks @kwilczynski.

[kwilczynski]

I am merely trying help :) I did not wanted to make it sound bad or horrifying, sorry. Fix is easy: check if String and if not empty.

[kbarber]

I think generally an input function name might need validation, even if Puppet::Parser::Functions.function() does kind of the right thing anyway - it really should return a second indicator if the function name is invalid (not just if it exists/doesn't exists) ... however I'm wary of replicating validation for function names here in this function. Better to let Puppet core do this kind of deeper validation IMHO so the rules are kept in one place.

But string and not empty is a damn good start.

[kbarber]

The code is good made can't find much fault (although you don't have a space before the require 'spec_helper' you heathen!!! :-).

Umm ... what about putting an 'is' in front of this? I figure it lines up with the other is_something_or_rather functions we have around.

[kwilczynski]

@kbarber +1 for prefixing with "is_".

[arioch]

Awesome work guys.
Can't wait to see this one get merged in.

[adrienthebo]

Merged into master as fc2352f.

Thanks again for the contribution!

-Adrien