Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Oct 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
530b8c3
(bug) - Remove reference to obselete manifest_dir
jordanbreen28 Oct 9, 2023
4e88604
(maint) - Add stricter rspec dependency
jordanbreen28 Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/puppetlabs_spec_helper/module_spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ def verify_contents(subject, title, expected_lines)

c.environmentpath = spec_path
c.module_path = module_path
c.manifest_dir = File.join(fixture_path, 'manifests')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jordanbreen28 jordanbreen28 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# https://github.com/puppetlabs/rspec-puppet#strict_variables
c.strict_variables = ENV['STRICT_VARIABLES'] != 'no'
Expand Down
2 changes: 1 addition & 1 deletion puppetlabs_spec_helper.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ Gem::Specification.new do |spec|
spec.add_runtime_dependency 'puppet-lint', '~> 4.0'
spec.add_runtime_dependency 'puppet-syntax', '~> 3.0'
spec.add_runtime_dependency 'rspec-github', '~> 2.0'
spec.add_runtime_dependency 'rspec-puppet', '>= 2.0'
spec.add_runtime_dependency 'rspec-puppet', '~> 4.0'

spec.requirements << 'puppet, >= 7.0.0'
end