Check if $is_pe exists before using it

[raphink]

$is_pe is not a standard fact, so the
current code fails with strict_variables on.

[raphink]

@mhaskel @cmurphy This is to fix #261 (comment)

[mcanevet]

+1

[daenney]

There has to be a better solution to this than sprinkling more if defined around in manifests.

[raphink]

Hopefully there is, I can't think of one right now.

[igalic]

imo, it should be part of the language (even c99 got that by now;) that

if (!foo) { /* error handling */ }

if foo is NULL, this will handle the error case correctly even on platforms where NULL is _not_ 0.
in my opinion, this

if $::is_pe {
  # do stuff
}

shows the intent of the programmer to check whether $is_pe is defined, and the compiler should do the check for us, implicitly.

[raphink]

In Ruby, if var fails if var is undefined, which is good.

[cmurphy]

What if we use if getvar('::is_pe') instead?

ping @hunner @mhaskel @cyberious

[raphink]

@cmurphy I'm fine with getvar() if that fixes it. The current situation breaks non-PE catalogs with strict_variables on.

[cmurphy]

We've merged #273 which should fix the issue, so closing this one.

[raphink]

LGTM, thanks

[raphink]

Actually, this solution seems to fail with the future parser:

error during compilation: Evaluation Error: Error while evaluating a Function Call, uncaught throw :undefined_variable 

[daenney]

@hlindberg Care to posit an opinion here?

[hlindberg]

There is a method called exist?(varname) on scope that checks if a variable exists or not. This can be used from Ruby. The function defined('$varname') is the Puppet equivalence.

A better solution in Puppet code is to use the $facts variable, since there is no error to lookup $facts[is_pe] if is_pe does not exist - it is simply undef.

[hlindberg]

The reason it fails with future parser is because it should fail. You cannot determine the existence by simply returning a value - so the function is flawed.

[raphink]

@hlindberg so would you recommend using $facts[is_pe] instead in this case? Will this only work in Puppet 3+?

[hlindberg]

hm, so it requires turning on other options (structured_facts IIRC). So, maybe you end up with two problems instead of two. Let me look at the defined function....

[hlindberg]

The defined function was updated for Puppet 3.6 in order to ask if a variable is defined (i.e. checking with scope.exist). This does not require future parser, but does not work for versions before 3.6. Uncertain when strict_variables was added - it was before 3.6.0 IIRC. This means that there is a gap for some 3x versions of puppet where users can turn on strict_variables but not have the updated defined function.

[hlindberg]

The code works for me on stable (3.7.5) - which puppet version was used when it failed? @raphink ?

Going forward (from puppet 4.0) I think it is best to use $facts to lookup facts. Before 4.0.0 that requires --immutable_node_data, or --trusted_node_data to set $facts. If you want to avoid that extra complication, and you are supporting 3.6.0 and forward only, then use defined. For the versions before defined was changed but after strict_variables was added there is no way to ask if a variable exists or not. If the fix should work for all 3x versions, the only way I can think of checking is by adding a function (or an inline ERB template) that performs the check in a way that always works.

Hope that helps.

[cmurphy]

Reverted #273 in order to merge this.

[raphink]

There is yet another option, which is to provide an is_pe fact with concat, with a low priority and a false value. I'm not a big fan of this option, but it would work in all cases. In this case, the fact could actually be provided by a dependent module so it can be used in various places.

[raphink]

Or yet another possibility: keep all PE specific logic out of concat and use parameters+defaults to setup concat in PE.

[igalic]

@hlindberg going forward, with puppet 4 being strict by default, i think we really need a workable solution for this pattern. i agree with @daenney that sprinkling our code with if defined is _not_ that solution. neither is, imo, getvar(). it should be something native to puppet, not a workaround in code that someone recognises as pattern and puts into a function in stdlib.

[hlindberg]

@igalic Puppet 4.0 does not have strict variables turned on by default - it is recommended to turn it on if you can, but generally too many existing modules break if it is on by default.

When you say "native to puppet" what do you mean? Do you want an operator for this purpose? Since the only really valid use case is to check if a fact is present then this should be done with $facts[fact_name] and that is on by default in Puppet 4.0.0.

The other use case is to mix in arbitrary data, that should be done with data in modules and looked up, not by checking if a variable is defined or not.

[raphink]

Upon thinking about it more, I really wonder if it is relevant to have PE-specific settings in generic modules like concat. It might just be better to expose command_path as a parameter to the concat class and override it in PE. Also, other orgs might want to change that, too.

[hlindberg]

@raphink I think you are on to something there - having a module that depends on a fact without introducing it or declaring a dependency on the module that introduces it is quite smelly.

[PierreR]

since there is no error to lookup $facts[is_pe] if is_pe does not exist - it is simply undef

@hlindberg looking up an hash with a key that might not exist is acceptable (and even encouraged) in the puppet ecosystem ?
If this is so I might try to persuade @bartavelle to support this behavior in the permissive mode of language-puppet (even if I know he hates it ;-).

[bartavelle]

@PierreR How can anyone like that ? :)

[hlindberg]

The convention that it is ok to lookup and get nil/undef/null/unit (or however undefined is encoded) is common in many programming languages for an open ended construct such as a Hash. If it feels smelly that keys may be missing, then do a type assertion on the Hash using the Puppet type Struct before accessing keys.

[bartavelle]

@hlindberg Only joking ;) It turns out that missing keys are always a bug in my manifests, but I understand it's very different for other people.

[PierreR]

The convention that it is ok to lookup and get nil/undef/null/unit (or however undefined is encoded) is common in many programming languages for an open ended construct such as a Hash. If it feels smelly that keys may be missing, then do a type assertion on the Hash using the Puppet type Struct before accessing keys.

Sorry in advance for expressing my personal opinion here ;-)

As a specialized DSL I would expect Puppet to be closer to the end user than the library authors. In that spirit the most logical default for h['akey'] seems to bark when akey does not exist.

Instead we force the end user to write expensive behavior tests so that they can get a minimum of confidence from their manifest (or we ask him to use a rather involved new syntax Struct).

If you look at it, many current usage of this idiom come (from library authors) out of laziness (the feature is half baked downstream) or out of a lack of a better solution (another primitive function that would silently fail when it is really what you want to do).

Anyhow thanks for the feedback. I do understand that such a change would break compatibility and might not be suitable, at least now.

[daenney]

Instead we force the user to write expensive behavior tests so that they can get a minimum of confidence from their manifest (or we ask to use a rather involved new syntax Struct).

You're mix-matching user as in 'end user of the module' and library author/developer across your argument which makes it rather difficult to follow what you mean.

As far as Struct goes, it's really not that hard and any developer of a Puppet module ought to familiarise itself with the type system on a basic level.

[bartavelle]

I think the meaning of @PierreR is that you are forced to choose between brittle and verbose for no perceived gain. I personally never took advantage of the fact that you silently get undef when no key exists, and I can't say I see a situation where I might like that. Moreover, even if they exist, I suppose they are likely to be much less common than the other way ...

[igalic]

i very, very, very frequently use undef as a "third state" in booleans for instance:

# Aggressive network behaviour: Use multiple TCP connections when
# writing archives.  Use of this option is recommended only in
# cases where TCP congestion control is known to be the limiting
# factor in upload performance.
<%- if @aggresssive_networking.nil? -%>
#aggressive-networking
<%- elsif !@aggressive_networking -%>
no-aggressive-networking
<%- else -%>
aggressive-networking
<%- end -%>

to only render absolutely minimal changes in files.