Revert "Add nginx_version fact"

[3flex]

Reverts #753

The original change is not compatible with earlier versions of Facter (#758). Need to either increase version dependency of Facter, or ensure the change is backwards compatible to Facter 1.7.

@jfryman what is your preferred course of action?

If we support Facter 1.7 then it must be covered by Travis tests too.

[jyaworski]

Honestly, why are we still supporting Facter 1.7?

Regardless of that answer, replacing Facter::Core::Execution with Facter::Util::Resolution and execute with exec should fix it.

[jfryman]

The problem with factor is that it doesn't seem to have an EOL date. I know that PL is now bundling, but that still doesn't help legacy installs.

I certainly don't want to alienate older installs. I propose adding Facter to the matrix, and if it passes, then let's just keep going.

@jyaworski could you issue a PR?

[jfryman]

Spoke right too soon. Just got word back... PL doesn't support 1.7. (h/t @stahnma) So, meh. I maintain we keep compatibility with anything supported by upstream.

[jyaworski]

@jfryman that's what VoxPupuli has been doing (mostly). Moving at upstream's pace.

[alex-harvey-z3q]

Whatever Puppet Labs say, Facter 1.7.0 was only released April 2013 and Facter 1.7.6 was released June 2014, and this Nginx module's lack of Puppet 4 support is one of the reasons we're not upgrading to Puppet 4 yet.

Is this module the right place for an Nginx version custom fact though? I think that sort of custom requirement belongs outside of shared Puppet Forge modules. The Puppet Labs Apache module doesn't provide an Apache version fact for instance.

@jyaworski Do you need custom version facts for other software or only Nginx?

[jyaworski]

@alexharv074 actually, it does.

https://github.com/puppetlabs/puppetlabs-apache/blob/master/lib/facter/apache_version.rb

I disagree that knowing the version of something installed is a 'custom requirement.' Templates can use it for formatting, parameters can use it for logic (like $::puppetversion), and it's just generally useful in my opinion.

Please ping the issues preventing you from upgrading to puppet4.

@jfryman if you need another collaborator on this module, I'll do it. I use it in production.

[alex-harvey-z3q]

Hi @jyaworski that's a little disingenuous because the author of that file, 6 days ago, is you.

From your Github history, it looks like you're on a mission to add version facts to every Forge module you use. What this means is you're breaking Puppet for people who have old versions of Facter in order to add a feature that helps you manage multiple old versions of your own software.

What are you actually using these version facts for? Is it reporting or are they inputs to conditional logic in your manifests?

I think you will find that thousands of people are still using Facter 1.7, and the documentation for this Nginx module states that Facter 1.7 is supported, and Puppet 4 is not. I used that documentation as the basis of choosing this module.

If I had been in your position, I would have rolled a custom module to add facts that track the versions of all the software packages I cared about. That will be more flexible, too, because you'll no doubt care from time to time about the versions of software that aren't delivered by Forge modules.

[alex-harvey-z3q]

@jyaworski

I see that the fact you added in the Puppet Labs-supported modules is Facter 1.7 compatible:

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

So, why don't we just do the same here? I'm happy to raise the PR if people are happy with this compromise.

[jyaworski]

For the Apache module, they tested against something using Facter 1.7 and I had to switch to the older methods. I don't write compatibility with code that I don't need to, generally. The Facter docs say that Facter::Util::Resolution is deprecated, so I used Facter::Core::Execution as recommended.

If anyone wishes to submit a PR to support Facter 1.7, they're welcome to do so.

[alex-harvey-z3q]

👍 I'll raise a PR.

[alex-harvey-z3q]

@3flex feel free to decline this PR.

[jyaworski]

Just for those that come upon this PR in the future, here are links to the Facter APIs, and some usage (as of 3.1).

http://www.rubydoc.info/github/puppetlabs/facter/Facter/Core/Execution
https://docs.puppetlabs.com/facter/3.1/fact_overview.html

As of right now, I couldn't find an official deprecation warning. However, most sources I've looked at with a small Google search point mostly to Facter::Core::Execution, although a few still point to Facter::Util::Resolution. If I can find an official warning, I'll post it here.

[alex-harvey-z3q]

Strangely enough the methods in Facter::Util::Resolution are marked as deprecated in the code but not in the public documentation.

[3flex]

Closing in favour of #762