Skip to content

ci(github-actions): ⚡ add caching of dependencies to improve test build times #343

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

Merged
merged 26 commits into from
Apr 8, 2022

Conversation

ericapisani
Copy link
Contributor

@ericapisani ericapisani commented Apr 5, 2022

Changes made

  • Add caching of global NPM cache (the .npm folder)
  • Add caching of node_modules within the individual test fixtures (e.g: plugins/test/fixtures/<test-name>/node_modules)
  • Replaced build matrix in workflow test.yml file with 2 jobs - build-ubuntu and build-mac
  • Added a conditional for running the build-mac job so that it only runs on the main branch
  • Add global install of netlify-cli step to errors of 'netlify not found' in tests
  • Pinned netlify/build. Though still not sure as to why, the most recent release (26.5.2) was leading to build errors occurring within the macOS test runner, and 26.5.1 continues to work as expected. The build errors are reproducible in a local environment as well and not specific to the test runners.

Which problem is this pull request solving?

Improving test run times of the repo. See the linked issue below for more details.

Example of test run prior to these changes
Example of test run after these changes

List other issues or pull requests related to this problem

Addresses #338

Describe the solution you've chosen

The 2 main parts of the solution are:

  • caching of dependencies
  • removing the macOS test build run from development branches

~1GB of data is being downloaded in every test build (based on logs showing over ~300MB from the global npm cache and ~700MB for the node_modules folders in all the plugin/test/fixture folders are being restored from cache) and mitigating that a bit should lead to better build times.

It should be noted that the cache limits for a repository is 10GB, and once that's been reached Github will begin evicting caches until the size drops below 10GB.

The macOS test runners on Github are also known to be on the slower side (example issue can be found here), and is taking on average 2-3x as long as the Ubuntu test runner.

While there's still value in running these tests, given the similarities between linux and macOS it seems reasonable to assume that any major issues will be raised within the ubuntu environment that would also affect macOS. As an added measure of safety we can continue to run the macOS test runner to ensure no regressions on the main branch rather than running it on every commit within a feature branch).

A consideration that was made was to use multiple workflow files in order to keep the original build matrix for main branch builds and to have a 'development' workflow file that includes just the ubuntu job as it's currently written. Given there are not too many combinations of os/node version in the build matrix though, it seemed a little bit extra and opted to just have a single file for the time being.

A side effect of the decision to not use a build matrix though is that there is the duplication of steps between the two jobs. I had considered making a composite action, in order to remove the duplication of the steps but it seemed to be a bit of over-engineering at the moment given this would be the only project using the functionality. If we find that we are repeating a similar pattern with respect to our test builds in other plugin projects then it might make sense to investigate that option further.

Describe alternatives you've considered

Replace the use of the npm cicommand with npm install

Because npm ci removes node_modules folders prior to running the install, one of the ideas was to transition to using npm install where npm ci has been used in order to reuse cached node modules that would be restored with each build.

However, one of the strengths of this command is that is will fail test builds if the dependencies listed in the package.json don't match with the package-lock.json, and not automatically install and update the lockfile as npm install does. For this behaviour, I opted to keep the use of npm ci where it's currently being used.

Related docs

Updating the build matrix to conditionally include/exclude 'macOS' to the os property based on the branch that triggered the workflow

I had attempted to use a conditional (both if and a matrix environment variable) to include/exclude the macOS os option based on the branch that had triggered the workflow.

However, the matrix doesn't like the use of if within it's configuration, and although I had seen a community support article in Github showing the use of an environment variable to control the inclusion/exclusion of a variable within the configuration, it wasn't working out.

I had also tried defining an environment variable in the workflow scope for use within the build matrix configuration rather than in an expression within the build matrix configuration itself, with no luck.

Using the --prefer-offline flag in npm installs running within the test fixture folders

The --prefer-offline flag bypasses staleness checks on the cached data, but if data is missing then it will request that data. Using this didn't have the impact that was hoped for in the npm installs within the test fixtures, so opted not to include it

Related docs

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • I have added tests (we are enforcing 100% test coverage).
  • I have added documentation in the README.md, the docs directory (if
    any) and the examples directory (if any).
  • The status checks are successful (continuous integration). Those can be
    seen below.

@netlify
Copy link

netlify bot commented Apr 5, 2022

Deploy Preview for netlify-plugin-gatsby-demo canceled.

Name Link
🔨 Latest commit 41c314f
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-gatsby-demo/deploys/624f0159445e020008e46bbd

Update path in cache-dependency-path, didn't look to be correct
Unknown change has led to issues with the 'netlify build' command used in tests
'npm ci' should be referencing it already
Remove --prefer-offline flag as it didn't lead to an improvement
also use ref_name from github context as it's a bit cleaner to read
@ericapisani ericapisani changed the title ci(github-actions): ⚡ add cache action to improve test build times ci(github-actions): ⚡ add caching of dependencies to improve test build times Apr 7, 2022
@ericapisani ericapisani marked this pull request as ready for review April 7, 2022 18:34
@ericapisani ericapisani requested a review from a team April 7, 2022 18:34
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

🎉

@kodiakhq kodiakhq bot merged commit 51ba7bc into main Apr 8, 2022
@kodiakhq kodiakhq bot deleted the ep-cache-dependencies-in-workflows branch April 8, 2022 12:59
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.

2 participants