(bug) - remove obselete manfiest dir config setting & require rspec-puppet 4.x

[jordanbreen28]

Summary

This PR removes references to the obsolete manifest_dir puppet
configuration setting which was removed in puppet 4.x and above.

This caused spec_helper to throw `undefined method `manifest_dir='` as the latest release of rspec-puppet removed
this setting.

Additional Context

Also tightened the dependency on rspec-puppet to ensure breaking changes are not consumed by default.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

• 🟢 Spec tests.
• 🟢 Acceptance tests.
• Manually verified.

[bastelfreak]

I think the title should be updated to something like "(bug) - remove obselete manfiest dir config setting & require rspec-puppet 4.x"

[ekohl]

I think this isn't an appropriate fix. Not everyone can update to a new major version (that drops Puppet < 7.11 support). A compatible fix is to use

if c.respond_to?(:manifest_dir)
  c.manifest_dir = File.join(fixture_path, 'manifests')
end

[jordanbreen28]

In most cases I would tend to agree with you @ekohl! But I believe the manifest_dir setting was dropped after Puppet 3.x, so not sure if this is something we should even keep compatibility with? Happy to hear more thoughts on this.

[ekohl]

Still, you raise the minimum version to 4.0 in a bugfix. That can be breaking for some users.

[jordanbreen28]

Understood. The gem has a explicit requirement of Puppet 7.x + here, so in this case we can safely say this shouldn't be breaking for any users. But thank you @ekohl for being so thorough - it is appreciated.

[ekohl]

Weird I missed that requirement, but thanks for pointing it out. I missed 5ef8ca1. Now that explains the other problems I had! It really shouldn't pull that in for my use case. In acceptance tests I don't want Puppet installed, but I do need the fixtures set up.

Just thinking out loud: isn't that line equivalent to spec.add_runtime_dependency 'puppet', '>= 7.0.0'? If so, it may be easier to see at a glance what the dependencies are.

Now I'm wondering: would it make sense to pull out fixture code to a separate gem?

[jordanbreen28]

Yeah I must say it definitely isn't the typical way to set dependencies. Although I'm wondering is that because spec.add_runtime_dependency will actually pull the puppet gem in, which is maybe not what you'd want to happen.
Interesting point about pulling out the fixture code, as it definitely is one of the large use cases of the spec_helper - might be something I can raise internally.

[ekohl]

Yeah I must say it definitely isn't the typical way to set dependencies. Although I'm wondering is that because spec.add_runtime_dependency will actually pull the puppet gem in, which is maybe not what you'd want to happen.

Hmm, that's a good point. I don't see Puppet listed in https://rubygems.org/gems/puppetlabs_spec_helper so this is something I need to read up on.

Interesting point about pulling out the fixture code, as it definitely is one of the large use cases of the spec_helper - might be something I can raise internally.

For context: it's https://github.com/voxpupuli/voxpupuli-acceptance/#fixtures

Right now it's low priority for me, but I was updating some modules for me and I saw the puppet gem was being pulled in for my acceptance test and I didn't know why. Might be unrelated though.

[ekohl]

Ok, I tracked it down. Taking from bundle exec gem dependency:

Gem puppetlabs_spec_helper-6.0.2
  mocha (~> 1.0)
  pathspec (>= 0.2, < 2.0.0)
  puppet-lint (~> 4.0)
  puppet-syntax (~> 3.0)
  rspec-github (~> 2.0)
  rspec-puppet (>= 2.0)

Gem puppet-syntax-3.3.0
  pry (>= 0, development)
  puppet (>= 5)
  rake (>= 0)
  rb-readline (>= 0, development)

So puppetlabs_spec_helper pulls in puppet-syntax and that pulls in Puppet.

[ekohl]

And some further digging: the fixture code to check for the puppet version also relies on the gem being present.