Confine nginx_version fact

[ekingme]

If this fact is not confined it will display a dialog on Windows which requires interaction. If puppet runs silently, it will hang.

Fixes #814

[igalic]

in theory, nginx can run on Windows: http://nginx.org/en/download.html

[danzilio]

Given the operating system support listed in https://github.com/jfryman/puppet-nginx/blob/master/metadata.json#L25-L59, this seems like a sane approach to me!

[3flex]

I'd rather have special behavior for Windows than white list individual *nix kernels, since some will inevitably be missed.

Can you please try the following on your Windows box? If you can also check the fact on any *nix boxes to make sure it still works that would be great. I don't have access to either at the moment.

I'm not sure if the Windows fact will be ignored because it returns nil... the docs say "If that resolution doesn’t return a value, Facter moves on to the next resolution" but I'm not sure what "doesn't return a value" means for nil. Assuming it stops processing on Windows once the first one returns nil we should be covered.

Facter.add(:nginx_version) do
  confine :kernel => 'Windows'
  setcode do
    return nil
  end
end

Facter.add(:nginx_version) do
  setcode do
    if Facter::Util::Resolution.which('nginx')
      nginx_version = Facter::Util::Resolution.exec('nginx -v 2>&1')
      %r{^nginx version: nginx\/([\w\.]+)}.match(nginx_version)[1]
    end
  end
end

[ekingme]

I was hopeful, but returning nil continued on to the next resolution.

[danzilio]

This should work:

Facter.add(:nginx_version) do
  setcode do
    if Facter.value(:kernel) == 'windows'
      nginx_version = nil
    elsif Facter::Util::Resolution.which('nginx')
      nginx_version = Facter::Util::Resolution.exec('nginx -v 2>&1')
      %r{^nginx version: nginx\/([\w\.]+)}.match(nginx_version)[1]
    end
  end
end

[ekingme]

@danzilio that works perfectly. Tested on Windows and CentOS 6.

[3flex]

Edit: Ignore the below, solution was posted above already.

---

Unfortunate - can you try this for me? If it works please rebase your branch on master since there are some fixes for tests in there, and hopefully the facter test will pass.

Facter.add(:nginx_version) do
  setcode do
    kernel = Facter.value('kernel')
    if kernel != 'windows' && Facter::Util::Resolution.which('nginx')
      nginx_version = Facter::Util::Resolution.exec('nginx -v 2>&1')
      %r{^nginx version: nginx\/([\w\.]+)}.match(nginx_version)[1]
    end
  end
end

[3flex]

@danzilio beat me to it! I'd drafted my response but hadn't sent it.

@ekingme please rebase and update with either @danzilio or my code so we can run the test we have for that fact.

[ekingme]

It did a combination of both your changes. I've tested on Windows and CentOS 6, with and without Nginx installed. It's working for me.

I pushed the update to this patch.

Facter.add(:nginx_version) do
  setcode do
    if Facter.value('kernel') != 'windows' && Facter::Util::Resolution.which('nginx')
      nginx_version = Facter::Util::Resolution.exec('nginx -v 2>&1')
      %r{^nginx version: nginx\/([\w\.]+)}.match(nginx_version)[1]
    end
  end
end

[3flex]

One last request: can you please squash to a single commit? Once done I'll merge right away.

Thank you!

[ekingme]

@3flex Done. Thanks!