Commit 0cd2a305 authored by micah's avatar micah

Fix fact for when shorewall is not yet installed.

When a node has puppet run for the first time, shorewall may not be
installed. In that case there are a few problems that appear in puppet4:

1. Warning: Facter: Could not retrieve fact='shorewall_major_version', resolution='<anonymous>': undefined method `split' for nil:NilClass

This is because running 'shorewall version' fails and so results in a
nil, and the split cannot be done on a nil. That is solved by first
running the 'shorewall version' and setting a variable. If that variable
is not nil, then we can split off of that

2. Error: Could not retrieve catalog from remote server: Error 500 on
SERVER: Server Error: Evaluation Error: Error while evaluating a
Resource Statement, Evaluation Error: Error while evaluating a Function
Call, 'versioncmp' parameter 'a' expects a String value, got Undef

This happens because the shorewall_version is set to Undef, but we need
to have it set to a String. So we set the variable to '-1' if it is not
parent b140aabf
Facter.add("shorewall_major_version") do
setcode do
Facter::Util::Resolution.exec('shorewall version').split('.').first || nil
shorewall_version = Facter::Util::Resolution.exec('shorewall version')
if shorewall_version != nil
shorewall_major_version = shorewall_version.split('.').first
shorewall_major_version = '-1'
  • this can be reduced frpm the if on to

    shorewall_version.nil? ? nil : shorewall_version.split('.').first

    • there is really no need to do the assignment
    • there is really no need to assign -1 if nothing is found. then we can just leave the fact nil
Please register or sign in to reply
  • Sorry, I'm not understanding, the code you cited was code I removed, maybe you mean something like this:

    (Facter::Util::Resolution.exec('shorewall version') || "").split('.').first || nil


  • Ok this would reduce everything to one line, which is very perl-ish to do and not that readable and actually it would return an empty string if fetching the version wouldn't work.

    What I meant is that the whole setcode part can be:

    setcode do
      shorewall_version = Facter::Util::Resolution.exec('shorewall version')
      shorewall_version.nil? ? nil : shorewall_version.split('.').first

    But while looking at it now, we could actually get 2 facts: shorewall_version && shorewall_major_version. The first one would be line 1 and the second one would take the value of the first fact and do the stuff we see here.

    # lib/facter/shorewall_version.rb
    Facter.add("shorewall_version") do
      setcode 'shorwall version'
    # lib/facter/shorewall_major_version.rb
    Facter.add("shorewall_major_version") do
      confine :shorewall_version => /\d/
      setcode do

    I did not test it, but this should give you the same.

  • I added these, but intrigeri reported in that they are running on all systems, even those not using shorewall and thus failing since shorewall doesn't exist. Maybe we can use a 'confine' to limit it somehow?

  • In my opinion intrigeri did not yet apply this patch and he is running into the problem that micah fixed.

  • also I realized we might want to make the regexp /\d+/ I guess

  • mentioned in commit 70bba045

    Toggle commit list
Markdown is supported
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment