Validate Puppet Version

[dhollinger]

closes #99

[ghoneycutt]

This project is meant to validate that the metadata is correct for a puppet module and the metadata is not invalid because it uses an unsupported version of puppet. I'm against adding this functionality at all. If it is added, there needs to be a simple way to disable it.

[bastelfreak]

@ghoneycutt this PR is based on a discussion we had yesterday in #voxpupuli. The idea was to check if the supported puppet version range is within the range that Puppetlabs supports.

[ekohl]

I'm fully with @ghoneycutt here. This will break things for me for and I'd like avoid this.

Now what I would much rather want is a check if the latest version is supported. That's much more important IMHO. It should still be configurable though.

[bastelfreak]

Making this a configureable feature is a good idea, thanks @ghoneycutt. On Vox Pupuli we agreed on only supporting the current Puppet versions that also Puppetlabs support, so a check for that would be helpful. This gem was the first that came to our mind as we thought about a good place to implement it.

[ghoneycutt]

@dhollinger I'm actually all for this functionality and would use it if it was in a separate project since it is not related to verifying if the metadata is correct.

[dhollinger]

I'll work on adding the configurable option later today/tonight

[ekohl]

I'll work on adding the configurable option later today/tonight

How about a setting that accepts the minimum version so users can set it to something like 4.8. It should also support something like current. That should detect the current version. It probably means we also need to prepare it for future EOL dates.

[dhollinger]

@ekohl @rnelson0 I'm all for making it configurable, but as a validation tool, I don't think we should allow the lower bound limit to be set. It sets a precedent that we will support options for EOL/Unsupported versions of Puppet. I think it should be a simple on/off based option like the --strict-supported-versions suggested option.

I'll do whatever the community as a whole agrees on, but we don't have the man power to support every potential option a user needs. The larger user base still has Puppet 2 users and we definitely should not be adapting code to support Puppet 2 or 3 and I'd extend that to anything below Puppet 4.10. Rather just let them disable strict checking.

[rnelson0]

This isn't about our support, it's about users validating their own modules. What "we" support would be defined in the default, but if I am stuck at 4.2 at work, I should be able to specify that. It won't reflect on VP at all.

[dhollinger]

I guess the argument I'm trying to make is that testing tools should not account for every potential option. If it did, the rspec, rspec-puppet, cucumber, etc would have to support options for every version of Puppet/RSpec/Ruby ever because a user might still be using an old version. We should not support/enable that, instead it should be a simply on/off option, or, if they want to continue using an older version of software, they have to use the older versions of tools that go with that.

[ekohl]

I agree with @rnelson0: I see metadata-json-lint as a generic tool, not a voxpupuli tool. Those settings are meant for users while voxpupuli will probably just stick with supported versions.

[llowder]

This may require adding some functionality to make metadata-json-lint pluggable (like puppet-lint is for checks), but would it be acceptable to have this check be part of core as is - with a simple way to disable it via options, and then a separate plugin made available that allows a custom range (just lower, lower - upper, just upper) to be specified for the versions?

[dhollinger]

@rnelson0 @ekohl @ghoneycutt

In relation to the "problem" this PR is trying to solve, I think that for the initial roll out of something like this should focus on being simple initially. So, add the functionality to validate strict-requirements for puppet versions as default with the ability to disable it.

We can return to the discussion of adding the ability to add a lower (and/or upper) bound later as a separate PR or as the start of a plugin system for metadata lint as per @llowder's suggestion.

Thoughts?

@bastelfreak @binford2k - anything to add?

[binford2k]

Nothing to add, but this has been a fascinating conversation to watch :D

[bastelfreak]

@dhollinger I agree with your and @binford2ks last statement.

[dhollinger]

@ghoneycutt @ekohl @rnelson0 - I've updated the PR with a few changes based on the discussion:

• Added a --[no-]strict-puppet-version option to the gem and set it to default to false.
• Updated the output when the strict-puppet-version is turned on it now outputs a WARN instead of an ERROR.
• Renamed the related test from requirements_bad_version to requirements_eol_version.
• Added Ruby 2.4.3 back into the travis matrix since that ships with the more recent Puppet Agent packages. Kept 2.4.4 since that is what the Puppet Development Kit will test on by default.

@llowder @binford2k @bastelfreak - your input is welcome as well.

[dhollinger]

@ekohl I will work on having the requirements section of the metadata.json file use the existing ruby class(es) to parse and validate the semver of the requirements section. I think that there should either be a strict-puppet-semver option that enforces proper semver format, or it should always be enforced as the puppet version will always be in semver format unless a user is building their own puppet (which at that point they're probably not using this gem).

As for minimum version, I still think that should be on/off only (defaulted to off). Hard-coding the minimum puppet version is no different than what other gems/libraries would do for their validation and just gets updated as that matrix shifts. I highly doubt very many users that use this gem will care about minimum version beyond what the "supported" it or isn't. Everything else is an edge case and it is not a good idea in any way for us to try support a bunch of edge cases as they are just that, outside of what the norm is.

I think @llowder's suggestion is better for that as it would allow VP or users to create their own plugins to add functionality for such edge cases. I treat this as I would Chef's metadata.rb or RubyGems *.gemspec files. It's validating a file descriptive of the codebase and linting should be checked or enforced based on how the tools (like puppet module install, puppet forge, and/or r10k) utilize that file rather than based on potential edge cases.

[ekohl]

I think @llowder's suggestion is better for that as it would allow VP or users to create their own plugins to add functionality for such edge cases. I treat this as I would Chef's metadata.rb or RubyGems *.gemspec files. It's validating a file descriptive of the codebase and linting should be checked or enforced based on how the tools (like puppet module install, puppet forge, and/or r10k) utilize that file rather than based on potential edge cases.

I agree very much with this: if metadata-json-lint guarantees tools like puppet, forge and r10k accept your metadata as you intended it then that's a great scope. Opinions about content are controversial which makes it hard to reach a consensus as can be seen by this PR. Plugins can be a great solution.

[dhollinger]

@ekohl I'd like this PR to simply be for a simple on/off switch for enforcing strict puppet versions (Supported version + proper SemVer) only and is defaulted to off. Any additional functionality I'd like to push to a discussion about a plugins system for metadata.json.

Additionally, if paired with disabling fail-on-warnings a user can use this one option to check if their puppet version is still supported without failing a pipeline/build.

Thoughts?

[dhollinger]

Squashed commits into single commit

[ekohl]

@ekohl I'd like this PR to simply be for a simple on/off switch for enforcing strict puppet versions (Supported version + proper SemVer) only and is defaulted to off. Any additional functionality I'd like to push to a discussion about a plugins system for metadata.json.

I don't like building in technical debt from the start. Hardcoding values is a bad engineering practice. They should at least be defined as constants at the top of the class. That means it's very easy for the maintainer to change the values, even if the user can't just now.

[dhollinger]

@ekohl I can agree with that. Being in a constant makes it easier to find when it needs to be changed as well.

[dhollinger]

@ekohl any other comments or concerns?