(breaking) - Remove build rake tasks

[jordanbreen28]

Summary

this removes the build rake tasks from puppetlabs_spec_helper, as
it does not make sense to have these exist in a gem which sole purpose
is to help unit testing.

Additional Context

Add any additional context about the problem here.

• Root cause and the steps to reproduce. (If applicable)
• Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.95%. Comparing base (6e600d1) to head (f9878f8).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
+ Coverage   41.74%   43.95%   +2.21%     
==========================================
  Files          10       10              
  Lines         678      662      -16     
==========================================
+ Hits          283      291       +8     
+ Misses        395      371      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bastelfreak]

looks good to me. Do you want to keep this open until we've more breaking changes?

[jordanbreen28]

@bastelfreak yes please, just until we get more eyes!

[ekohl]

I'd just drop them altogether since there's already the same code in puppet-blacksmith to provide module:build:
https://github.com/voxpupuli/puppet-blacksmith/blob/3f187bafe5c5785ff6bafe9f9ca06410109e0b03/lib/puppet_blacksmith/rake_tasks.rb#L57-L63

This was done in voxpupuli/puppet-blacksmith@f955a02 about 4 years ago.

[ekohl]

I still lean to complete removal. I think most people rely on puppet-blacksmith and that already has the integration

[bastelfreak]

I agree with @ekohl here. puppetlabs_spec_helper could also just include puppet-blacksmith, so users don't have to do it?

[jordanbreen28]

@ekohl @bastelfreak yeah I realised that after I updated, the joys of working before you've had your morning coffee. I suppose no time like the present to implement a big change like this, I'm going to raise it with the team.

[ekohl]

could also just include puppet-blacksmith, so users don't have to do it?

I'm not sure I agree with this. If you look at the name it's the spec helper: designed for (unit) tests. Not the release helper.

With Vox Pupuli we put the release gems in their own group so you can install a more minimal set of gems. If puppetlabs_spec_helper depends on puppet-blacksmith that becomes impossible.

[jordanbreen28]

@ekohl after some discussion with the team, we agree with your point. The build rake tasks do not really belong here in puppetlabs_spec_helper, and as such I'm going to update this PR to action there removal (thanks for the help with that!).

Moving forward, our next item is to add a build rake task to puppet-modulebuilder. We appreciate that puppet-blacksmith includes this already, but believe for our use-cases it makes sense for us to rely upon our own tooling.

[ekohl]

I started to extract all the fixture handling to https://github.com/ekohl/puppet_fixtures too, but never got around to completing it. My intention with that was that puppetlabs_spec_helper becomes a really pure combination of building blocks. For Vox Pupuli I was hoping to remove puppetlabs_spec_helper from our toolchain entirely.

My reasoning is that I need fixtures in our acceptance tests, but puppetlabs_spec_helper is mostly focused on unit tests. Once you extract fixtures, there's really very little left in this gem that we need.

[ekohl]

@ekohl after some discussion with the team, we agree with your point. The build rake tasks do not really belong here in puppetlabs_spec_helper, and as such I'm going to update this PR to action there removal (thanks for the help with that!).

Note this gem already loads in puppet-blacksmith if available:

puppetlabs_spec_helper/lib/puppetlabs_spec_helper/rake_tasks.rb

Lines 20 to 24 in 6e600d1

	begin
	require 'puppet_blacksmith/rake_tasks'
	rescue LoadError
	# ignore
	end

You can easily end up defining the task twice. I think rake tasks are additive, so you could end up building the module twice.

Since it's a soft dependency, you can't declare you're incompatible with certain versions. Luckily puppet-blacksmith does depend on puppet-modulebuilder so you could make it an incompatible version bump there and update puppet-blackmsith accordingly.

So it's not impossible, but it's not as trivial as adding a new method.

[jordanbreen28]

Note this gem already loads in puppet-blacksmith if available:

You can easily end up defining the task twice. I think rake tasks are additive, so you could end up building the module twice.

Since it's a soft dependency, you can't declare you're incompatible with certain versions. Luckily puppet-blacksmith does depend on puppet-modulebuilder so you could make it an incompatible version bump there and update puppet-blackmsith accordingly.

So it's not impossible, but it's not as trivial as adding a new method.

@ekohl those are all valid points, thanks for going through all that.
From what I have gathered, it may just be better to leave puppet-blacksmith here as a soft dependency, that way the tasks it offers will be available to users if the gem is present.

So my thinking here is:

1. Remove build tasks from spec_helper (as we have done)
2. Keep soft dependency of puppet-blacksmith in puppetlabs_spec_helper. As you have already mentioned, puppet-blacksmith does not make sense as a hard dependency here..
3. For PDK users, we can add puppet-blacksmith to the pdk-templates so the build tasks will still be present for users who use pdk to manage their modules.

This way we indirectly consume modulebuilder, the rake tasks supplied by puppet-blacksmith and prevent any duplicated logic and/or rake tasks.

Does this sound more inline with what you're thinking?

[ekohl]

Does this sound more inline with what you're thinking?

Yes, very much.