Puppet: System Administration Automated

Support

Ticket #1177 (closed enhancement: fixed)

Opened 6 months ago

Last modified 5 months ago

You should be able to test within a template for whether a variable or fact is defined

Reported by: micah Assigned to: luke
Priority: normal Milestone: 0.24.5
Component: language Version: 0.24.4
Severity: minor Keywords: template facter fact nil
Cc: Triage Stage: Ready for checkin
Attached Patches: Code Complexity: Easy

Description

If you create a fact that does not have a value for some class of hosts, then there is no variable available to the template at all (rather than the variable existing and returning 'nil'), so if you test for that fact in a template and a node that doesn't have a value for that fact runs, an exception is raised (because method_missing cannot find a variable).

The only way to test for an empty/nil fact value is by using @scope.lookupvar("variable"), false) which is a bit of black magic and that is not a good way to go about because that relies on the implementation details of the Template Wrapper? class.

The exception being raised is probably there to catch common cases, like people screwing up a fact value in a template.

Holoway looked at things on IRC and he thinks that the Puppet::Parser::Template Wrapper? class is doing the right thing, it just needs the equivilant of either has_fact? or exists? As a possible patch, he suggested the following: 

diff --git a/lib/puppet/parser/templatewrapper.rb b/lib/puppet/parser/templatewrapper.rb
index 13823d4..47857c4 100644
--- a/lib/puppet/parser/templatewrapper.rb
+++ b/lib/puppet/parser/templatewrapper.rb
@@ -19,6 +19,14 @@ class Puppet::Parser::TemplateWrapper
             @scope.parser.watch_file(@file)
         end
     end
+
+    def has_fact?(fact)
+      if @scope.lookupvar(name.to_s, false) == :undefined
+        false
+      else
+        true
+      end
+    end

     # Ruby treats variables like methods, so we can cheat here and
     # trap missing vars like they were missing methods.

Attachments

0001-Adding-has_variable-support-fixing-ticket-1177.patch (3.9 kB) - added by adamhjk on 05/13/08 06:44:55.
Adds has_variable?() support to Puppet::Parser::Template Wrapper?

Change History

04/04/08 04:06:43 changed by Fujin

  • patch changed from None to Insufficient.
  • severity changed from normal to minor.
  • stage changed from Unreviewed to Needs design decision.
  • milestone set to unplanned.

I feel this is requiring of Luke's input; that is, should the fact evaluation be done in Puppet (with a case/if) or should the templatewrapper support it with this or a similar patch.

The supplied code needs a test written, also.

04/04/08 17:24:09 changed by luke

  • summary changed from if a fact does not evaluate, you cannot test for that in a template to One cannot test within a template for whether a variable or fact is defined.
  • stage changed from Needs design decision to Needs more info.

Well, the first point that must be made is that the compiler doesn't distinguish between facts and other variables. It would be much more correct to call the method 'has_variable?', or even 'variable_defined?'

Second, I guess I'm fine with the general idea, but you can't actually set variables to 'nil' in Puppet, so it's just as effective to say 'variable.nil?', which makes me think this is a touch overkill.

Is there something about 'variable.nil?' that doesn't work for you?

04/07/08 22:09:35 changed by adamhjk

Makes sense that it should be has_variable?.

I don't think it is as effective to say variable.nil?. The issue here is you are using method_missing in Puppet::Parser::Template Wrapper?: 

    # Ruby treats variables like methods, so we can cheat here and
    # trap missing vars like they were missing methods.
    def method_missing(name, *args)
        # We have to tell lookupvar to return :undefined to us when
        # appropriate; otherwise it converts to "".
        value = @scope.lookupvar(name.to_s, false)
        if value != :undefined
            return value
        else
            # Just throw an error immediately, instead of searching for
            # other missingmethod things or whatever.
            raise Puppet::ParseError,
                "Could not find value for '%s'" % name
        end
    end

So anything that calls a variable that isn't returned from lookupvar is going to generate an exception. Which is the right behavior, since you want the template to explode if you fat-finger a variable name.

I haven't tested it, but I'm pretty sure variable.nil? is going to explode.

Regarding tests, I don't see any tests for Template Wrapper? at all -- am I missing something?

04/08/08 18:10:54 changed by luke

  • complexity changed from Unknown to Easy.
  • stage changed from Needs more info to Accepted.

As we discussed on IRC, Template Wrapper? is currently considered a hidden class, so it's not directly tested; all of the related tests are in the functions tests.

04/24/08 08:14:13 changed by luke

  • component changed from library to language.
  • summary changed from One cannot test within a template for whether a variable or fact is defined to You should be able to test within a template for whether a variable or fact is defined.

05/13/08 00:07:39 changed by luke

  • milestone changed from unplanned to 0.24.5.

It'd be great to get this into 0.24.5, since it's been coming up a lot more.

05/13/08 06:44:55 changed by adamhjk

  • attachment 0001-Adding-has_variable-support-fixing-ticket-1177.patch added.

Adds has_variable?() support to Puppet::Parser::Template Wrapper?

05/13/08 06:47:09 changed by adamhjk

  • owner changed from community to luke.
  • stage changed from Accepted to Ready for checkin.
  • patch changed from Insufficient to Code.

The attached patch fixes this bug, and adds full test coverage to Puppet::Parser::Template Wrapper?.

05/13/08 07:19:27 changed by adamhjk

At fujin's request, you would use this in a template with:

<% if has_variable?("something") -%>

.. do something if true

<% end -%>

05/13/08 18:58:47 changed by luke

  • status changed from new to closed.
  • resolution set to fixed.

Merged.