(1093) - Fix unresolved fact error

[jordanbreen28]

Fix for #1093

Prior to this PR, work was carried out on this module to update all instances of the now deprecated Facter::Util::Resolution, and replace all with its newer and supported counterpart Facter::Core::Execution.
However, these do not behave exactly the same.

Facter::Util::Resolution initially ran a which to locate the binary before executing, preventing any errors from occuring. The newer Facter::Core::Execution method did not do this, instead it attempted to execute.

This commit aims to introduce an "on_fail:false" flag to each execute statement, so that a failed execute will return false (boolean) and not generate an error.

[david22swan]

LGTM

[rtib]

Wouldn't it be better to confine the evaluation of facts to those nodes having the required commands installed? IMO, nodes haven't installed iptables are not required to report firewall facts at all.

[kjetilho]

Wouldn't it be better to confine the evaluation of facts to those nodes having the required commands installed? IMO, nodes haven't installed iptables are not required to report firewall facts at all.

There is no support for such a confine as far as I know. The alternative is to run some kind of which function, but even that should happen within the setcode block, so that the test is not run when the other confines are not satisfied. (In this case, kernel=Linux.)

[rtib]

You can confine a fact to a command or there is a confine block also available. I do such facts like, e.g.

Facter.add(:cassandra_info, type: :aggregate) do
  nodetool = ''
  confine do
    nodetool, _err, status = Open3.capture3('nodetool info')
    status.success?
  end

  chunk(:dc) { { dc: nodetool.match(%r{^Data Center *: (.*)$})[1] } }
  chunk(:rack) { { rack: nodetool.match(%r{^Rack *: (.*)$})[1] } }
end

In the case the command fails or cannot be executed, the confine block returns false and the fact won't be reported at all, while there isn't even a warning on the agent.

[jordanbreen28]

You can confine a fact to a command or there is a confine block also available. I do such facts like, e.g.

Facter.add(:cassandra_info, type: :aggregate) do
  nodetool = ''
  confine do
    nodetool, _err, status = Open3.capture3('nodetool info')
    status.success?
  end

  chunk(:dc) { { dc: nodetool.match(%r{^Data Center *: (.*)$})[1] } }
  chunk(:rack) { { rack: nodetool.match(%r{^Rack *: (.*)$})[1] } }
end

In the case the command fails or cannot be executed, the confine block returns false and the fact won't be reported at all, while there isn't even a warning on the agent.

Thanks for the suggestion @rtib, when implementing a confine statement such as

confine { Facter::Core::Execution.which('iptables') }

this introduces spec test failures on nodes that do not have the package installed.

[kjetilho]

Thanks for the suggestion @rtib, when implementing a confine statement such as

confine { Facter::Core::Execution.which('iptables') }

this introduces spec test failures on nodes that do not have the package installed.

You need to mock the .which method, too:

      allow(Facter::Core::Execution).to receive(:which).with('iptables')
                                                         .and_return('/usr/sbin/iptables')

This worked in my rspec environment without iptables installed.

[rtib]

I assume you mean spec checks like spec/unit/facter/iptables_persistent_version_spec.rb#L34. That's because, these checks assume the fact being reported contain a nil, but a confined fact won't be reported. On these nodes the spec should expect the fact missing.

IMO, this would be the desired behaviour. Imagine an environment running a large number of nodes, but only a few need firewall. There is no need to report facts with value nil from nodes which doesn't configure a firewall. This could also reduce the utilisation of PuppetDB. It is not a big difference within the DSL if a fact doesn't exists or exists but its value is unset, since for those nodes the firewall module is not included in their manifest.

[jordanbreen28]

Changes applied, thanks @rtib & @kjetilho!

[LukasAud]

LGTM, GJ

[canihavethisone]

Excellent collaboration here to address the issue quickly and professionally, thanks all!