Move npm install outside of Docker #1328
Merged
Conversation
This file contains 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
- moved style guide lower in developer.md - moved section on branch workflow to contributing.md
Remove the npm-cache docker image
optimize the composer install step - should not be coupled with apt install
renamed tests.yml to pull_request.yml In addition to running tests, we should also build the app and ensure it builds :)
This changes the build to require node on the developer's machine and moves the npm install outside docker (to take advantage of npm caching) Additional notes: - I needed to rebuild node-sass in Docker since the Docker environment is likely different than the developer host machine - I removed the npm run build:dev from test-e2e because I discovered it's not needed
Add the npm install step to both the build-and-publish-adhoc.yml and integrate-and-deploy.yml files
rmunn
approved these changes
Mar 14, 2022
megahirt
commented
Mar 14, 2022
There was a problem hiding this comment.
great work @megahirt , I'll trust you guys that the make and make dev still work from a "power-washed" env. I take it you had fun doing this 😊 future devs will never know how much faster things are now!
|
@longrunningprocess I didn't actually test that scenario but will try that tomorrow. I assume it does since it is building on GHA ok. |
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.
Description
This changes the build to require node on the developer's machine and moves the
npm installstep outside docker (to take advantage of npm caching). This should result in faster Docker builds on the developer's machine and possibly also on GHA (due to node_module caching on GHA)Additional notes:
npm run build:devfrom the test-e2e Dockerfile because I discovered it's not neededImportant Note to Developers
This changes the build in a significant way. Once this PR is merged, the developer machine must have the appropriate version of node and npm installed locally. Use Node 14.16.1 and npm 7.6.3
Fixes #1323
Type of Change
Checklist