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.

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

Download forge modules in parallel #284

Merged
merged 1 commit into from
Oct 30, 2019
Merged

Conversation

logicminds
Copy link
Contributor

This improves the forge module download speed by at least 400% when multiple modules are specified in fixtures.yml. Git repositories were previously already using parallel downloads so this only fixes modules.

This also refactors the fixtures code by adding almost everything into a method and placed inside the fixtures module spec module.

@logicminds
Copy link
Contributor Author

Looks like my editor changed a bunch of markdown stuff too with the vscode markdown plugin.

@puppetcla
Copy link

Waiting for CLA signature by @logicminds

@logicminds - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

@logicminds
Copy link
Contributor Author

Not sure what is wrong the the CLA bot but I already signed so I can't sign twice.

Also rubocop is failing without my changes.

@logicminds logicminds force-pushed the parallel branch 2 times, most recently from cbf42d7 to 374160f Compare March 27, 2019 21:50
@codecov-io
Copy link

codecov-io commented Mar 28, 2019

Codecov Report

Merging #284 into master will increase coverage by 0.74%.
The diff coverage is 20.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
+ Coverage   44.88%   45.62%   +0.74%     
==========================================
  Files          11       11              
  Lines         782      789       +7     
==========================================
+ Hits          351      360       +9     
+ Misses        431      429       -2
Impacted Files Coverage Δ
lib/puppetlabs_spec_helper/tasks/fixtures.rb 38% <20.68%> (+2.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0797846...bd7cb75. Read the comment docs.

@logicminds logicminds force-pushed the parallel branch 2 times, most recently from 95aad7d to bd7cb75 Compare March 28, 2019 00:48
@puppetcla
Copy link

CLA signed by all contributors.

Gemfile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@baurmatt
Copy link
Contributor

This look really promising! :) What needs to be done to get this merged?

It looks like the failing test is due to a Travis error and unrelated to the code changes.

@logicminds
Copy link
Contributor Author

Probably just a rebase, and a slight code change to accommodate older puppet. I'll fix this and ping the maintainers.

  * previously only the git repositories were downloaded
    in parallel.  This saved a ton of time but forge modules
    were still downloaded in serial.
  * This adds the same parallel download feature of the repositories
    to the forge modules.
  * Refactors a few methods so they can be run in parallel.
  * Extracts repository download method into more generic method
    so we can reuse it with forge downloads.
  * Typical speed improvments will be at least 400% with default of 10 threads.
    The more modules defined the better it gets.
  * Moves all methods to the fixtues helper module
  * remove condition logic to or assignments
  * move fixture references to cached value methods
  * moves download logic to individual methods
  * Forces rubocop to version 0.49
  * previously anybody with rubocop gem younger than 0.49
    would have issues with rubocop tests.  This was due to needing
    features in 0.49 and not enforcing that version.
Copy link
Contributor

@DavidS DavidS left a comment

Choose a reason for hiding this comment

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

looks good to me!

can we get a merge from @puppetlabs/pdk please?

@rodjek rodjek merged commit 3a0df02 into puppetlabs:master Oct 30, 2019
@logicminds logicminds deleted the parallel branch October 30, 2019 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants