generated from netlify/build-plugin-template
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add caching of project root node_modules
✅ Deploy Preview for netlify-plugin-gatsby-demo canceled.
|
Update path in cache-dependency-path, didn't look to be correct
This reverts commit ab8d9d1.
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
ascorbic
approved these changes
Apr 7, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes made
.npm
folder)node_modules
within the individual test fixtures (e.g:plugins/test/fixtures/<test-name>/node_modules
)test.yml
file with 2 jobs -build-ubuntu
andbuild-mac
build-mac
job so that it only runs on themain
branchnetlify-cli
step to errors of 'netlify not found' in testsnetlify/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, and26.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:
~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 theplugin/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 ofos
/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 ci
command withnpm install
Because
npm ci
removesnode_modules
folders prior to running the install, one of the ideas was to transition to usingnpm install
wherenpm 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 thepackage-lock.json
, and not automatically install and update the lockfile asnpm install
does. For this behaviour, I opted to keep the use ofnpm 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 workflowI had attempted to use a conditional (both
if
and a matrix environment variable) to include/exclude themacOS
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 innpm install
s running within the test fixture foldersThe
--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 thenpm install
s within the test fixtures, so opted not to include itRelated docs
Checklist
Please add a
x
inside each checkbox:README.md
, thedocs
directory (ifany) and the
examples
directory (if any).seen below.