(GH-234) Remove PDK dependency #235

glennsarti · 2020-01-28T03:49:27Z

Block on release of the puppet-modulebuilder gem

Previously Litmus depended on the entire PDK gem, however it only used the
module building capability. This commit

Adds the puppet-modulebuilder gem into Litmus.
Modified the rake helper to call the new Module Builder
Modifies the gemspec to remove PDK and add the puppet-modulebuilder gem

TODO

Have a release of puppet-modulebuilder gem
~~Drop the d DROP ME commit which is only there for testing this PR~~

DavidS

great to see this work started, it'll make the litmus install lighter and more robust.

What I'm unclear on: I thought we were talking about putting the ModuleBuilder in a external gem so it can be re-used by e.g. puppet-blacksmith. Not the least I'd also feel uncomfortable maintaining this code and its compatibility with the rest of the ecosystem. Did something change in that thinking?

DavidS · 2020-01-28T08:26:34Z

PS: where does the GH-xyz syntax come from? it's not linking automatically to #234 and that made me miss the background description on the first go-around.

glennsarti · 2020-01-28T10:09:50Z

Heya @DavidS

TLDR; Prove the code and use-case in Litmus now, getting fast feedback. In preparation to make it a gem.

What I'm unclear on: I thought we were talking about putting the ModuleBuilder in a external gem so it can be re-used by e.g. puppet-blacksmith.

We still are, but the work is a while off, at least for the PDK maintainers team. Thinking is

Make litmus more compatible now instead of later. Litmus is basically pulling in 15 gems (plus those gem's dependencies) for the sake of 323 LOC. Yes duplicating code can be bad, but it's not intrinsically bad. The extra gems seems like a heavy price to pay for something as (mostly) simple as tarring a directory
PDK currently uses beaker for package testing. Ideally we'd like to use Litmus. We can't (or shouldn't) really do that if Litmus pulls in PDK. This enables us (The PDK team) to consume Litmus
By getting the code into Litmus early, the PDK team can get faster feedback (aka use-in-anger) about what a Puppet Module Builder gem needs to look like. So we can release it quicker.

I'd also feel uncomfortable maintaining this code and its compatibility with the rest of the ecosystem.

I did consider this before I raised the PR, and it came down to balancing maintenance of somewhat duplicate code vs keeping the overhead of the PDK dependency for the sake of a one piece of functionality. What swayed me over was:

As part of moving it out of PDK into, and eventually, it's own library is to reduce the scope of the classes/modules. The S in SOLID. So the idea is there should be even less maintenance than if it was in the PDK itself.

The code in question hasn't had a bugfix since Jul 2019, and Oct 2018 before that. It's relatively stable. So in the short term (say, next 6 months) the maintenance overhead I expect would be practically zero.

Happy to have a chat about in person if you wish.

glennsarti · 2020-01-28T10:11:27Z

PS: where does the GH-xyz syntax come from? it's not linking automatically to #234 and that made me miss the background description on the first go-around.

I think I originally got it from an Open Source project within Puppetlabs. Maybe the windows modules way back when. It denoted the issue came from GitHub instead of Jira (MODULES- PUPPET- PDK-)

I think it pre-dated the #number syntax in github.

DavidS · 2020-01-28T10:23:25Z

Thanks for the in-depth response. All of that makes a lot of sense!

Since litmus is still very experimental[tm] and I'm about to set up a new gem for a different thing, I'd be happy to take the code from here and release it as APL2.0 puppetlabs/puppet-module-builder 0.1.0 in the next few days.

I even expect testing this code to be easier in a stand-alone repo.

glennsarti · 2020-01-28T12:33:27Z

Okay! As you can see from the TODO I still need to add the unit tests for this class. I know the acceptance tests have "proved" the code works, but I'd still like to have them.

Happy to work with you to put the code in the right spot!

ekohl · 2020-01-28T13:21:39Z

I'm fully with @DavidS on this and I've wanted a standalone gem for that part for some time, precisely for puppet-blacksmith. Also overall 👍 for dropping the PDK dependency in this gem.

DavidS · 2020-01-29T17:32:32Z

Created https://tickets.puppetlabs.com/browse/MODULES-10515 to get the infrastructure work scheduled in the team. might be a day or three until we get around to it.

glennsarti · 2020-02-12T02:35:52Z

@DavidS Just chasing this up

DavidS · 2020-02-12T08:58:42Z

@glennsarti @sheenaajay started on MODULES-10515 (neé IAC-532) yesterday.

Previously each spec file loaded and included individual test files, however this was not needed, and in some cases tests couldn't be run individually. This commit removes `load` commands as they're not required and moves the per-spec including to the spec_helper.

tphoney · 2020-02-27T09:13:01Z

❤️

codecov-io · 2020-02-27T13:27:00Z

Codecov Report

Attention: Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 10.00%. Comparing base (08bfbee) to head (19add0b).

Files with missing lines	Patch %	Lines
lib/puppet_litmus/rake_helper.rb	0.00%	10 Missing ⚠️
lib/puppet_litmus/rake_tasks.rb	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #235       +/-   ##
===========================================
- Coverage   55.63%   10.00%   -45.64%     
===========================================
  Files           7        7               
  Lines         665      840      +175     
===========================================
- Hits          370       84      -286     
- Misses        295      756      +461

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

lib/puppet_litmus/rake_helper.rb

ekohl · 2020-03-04T11:58:21Z

lib/puppet_litmus/rake_helper.rb

+  # @option opts [String] :module_dir The path of the module to build. If missing defaults to Dir.pwd
+  # @option opts [String] :target_dir The path the module will be built into. The default is <source_dir>/pkg
+  # @return [String] The path to the built module
+  def build_module(opts = {})


Any particular reason you're using an option hash instead of explicit parameters? I think explicit parameters would make the code easier to read.

was probably taken out from the pdk: https://github.com/puppetlabs/pdk/blob/master/lib/pdk/module/build.rb#L13
I'm happy with it either way, but it would be easier to read with parameters.

Previously Litmus depended on the entire PDK gem, however it only used the module building capability. This commit * Adds the puppet-modulebuilder gem into Litmus. * Modified the rake helper to call the new Module Builder * Modifies the gemspec to remove PDK and add the puppet-modulebuilder gem.

glennsarti requested a review from a team as a code owner January 28, 2020 03:49

glennsarti force-pushed the remove-pdk branch 2 times, most recently from e681610 to e37741c Compare January 28, 2020 05:37

DavidS added the bugfix label Jan 28, 2020

DavidS reviewed Jan 28, 2020

View reviewed changes

glennsarti added 2 commits February 27, 2020 09:23

(maint) Fix indenting

f031be0

glennsarti force-pushed the remove-pdk branch 3 times, most recently from f3bd778 to ef62b48 Compare February 27, 2020 02:00

florindragos reviewed Feb 27, 2020

View reviewed changes

lib/puppet_litmus/rake_helper.rb Outdated Show resolved Hide resolved

ekohl reviewed Mar 4, 2020

View reviewed changes

florindragos force-pushed the remove-pdk branch from 4fb96d6 to 351924a Compare March 4, 2020 12:10

florindragos changed the title ~~{WIP} (GH-234) Remove PDK dependency~~ (GH-234) Remove PDK dependency Mar 4, 2020

michaeltlombardi force-pushed the remove-pdk branch 2 times, most recently from f324df1 to 83ec12c Compare March 5, 2020 16:20

michaeltlombardi force-pushed the remove-pdk branch from 83ec12c to 19add0b Compare March 5, 2020 16:49

sanfrancrisko approved these changes Mar 5, 2020

View reviewed changes

sanfrancrisko merged commit c23e3df into puppetlabs:master Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(GH-234) Remove PDK dependency #235

(GH-234) Remove PDK dependency #235

glennsarti commented Jan 28, 2020 •

edited

Loading

DavidS left a comment

DavidS commented Jan 28, 2020

glennsarti commented Jan 28, 2020

glennsarti commented Jan 28, 2020

DavidS commented Jan 28, 2020

glennsarti commented Jan 28, 2020 •

edited

Loading

ekohl commented Jan 28, 2020

DavidS commented Jan 29, 2020

glennsarti commented Feb 12, 2020

DavidS commented Feb 12, 2020

tphoney commented Feb 27, 2020

codecov-io commented Feb 27, 2020 •

edited by codecov bot

Loading

ekohl Mar 4, 2020

florindragos Mar 4, 2020 •

edited

Loading

(GH-234) Remove PDK dependency #235

(GH-234) Remove PDK dependency #235

Conversation

glennsarti commented Jan 28, 2020 • edited Loading

DavidS left a comment

Choose a reason for hiding this comment

DavidS commented Jan 28, 2020

glennsarti commented Jan 28, 2020

glennsarti commented Jan 28, 2020

DavidS commented Jan 28, 2020

glennsarti commented Jan 28, 2020 • edited Loading

ekohl commented Jan 28, 2020

DavidS commented Jan 29, 2020

glennsarti commented Feb 12, 2020

DavidS commented Feb 12, 2020

tphoney commented Feb 27, 2020

codecov-io commented Feb 27, 2020 • edited by codecov bot Loading

Codecov Report

ekohl Mar 4, 2020

Choose a reason for hiding this comment

florindragos Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

glennsarti commented Jan 28, 2020 •

edited

Loading

glennsarti commented Jan 28, 2020 •

edited

Loading

codecov-io commented Feb 27, 2020 •

edited by codecov bot

Loading

florindragos Mar 4, 2020 •

edited

Loading