-
Notifications
You must be signed in to change notification settings - Fork 88
update GitHub Actions for vmui workflows: optimize caching and dependency installation process #1019
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
base: master
Are you sure you want to change the base?
Conversation
7f7a65c to
841c576
Compare
| vmui-builder-image -c "npm install && $(NPM_COMMAND)" | ||
| vmui-builder-image -c "npm ci && $(NPM_COMMAND)" | ||
|
|
||
| vmui-install: | ||
| NPM_COMMAND="true" $(MAKE) vmui-run-npm-command |
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.
I don't get it. Every time CI runs make vmui-lint/vmui-test/vmui-typecheck, it runs npm ci again anyway. Since npm ci always does a clean install, it doesn't really use the cache. Am I missing something?
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.
Yeah, you're right, changed it to npm i, because it takes the existing node_modules folder and validates it.
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.
I have two concerns...
What's the goal of these commands? running them manually or improving CI speed? If it's the latter, then npm i still runs 3x for lin, test, typecheck (not too long, but not too short either, and it still takes time), right? You might consider adding a VMUI_SKIP_INSTALL env var if these commands are also used manually.
If you change the workflow to run npm i only when needed (after resolving the above), then it's better to use npm ci (since it's faster and more secure). So the usual practice is:
- cache hit -> skip install (don't run
npm iornpm ciat all) - cache miss -> run
npm cionce -> cache node_modules
…and dependency installation process
841c576 to
284daa9
Compare
| uses: actions/cache@v5 | ||
| - name: Restore node_modules cache | ||
| id: cache-restore | ||
| uses: actions/cache/restore@v5 |
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.
I guess a single uses: actions/cache@v5 step is enough and simpler. From what I understand about actions/cache, it also saves the cache automatically at the end of the job if the key wasn't hit
If the provided key matches an existing cache, a new cache is not created and if the provided key doesn't match an existing cache, a new cache is automatically created provided the job completes successfully.
So it's enough to do this:
- name: Cache node_modules
id: cache
uses: actions/cache@v5
with:
path: app/vmui/packages/vmui/node_modules
key: vmui-deps-${{ runner.os }}-${{ hashFiles('app/vmui/packages/vmui/package-lock.json') }}
- name: Install dependencies
if: steps.cache.outputs.cache-hit != 'true'
run: make vmui-install| key: vmui-artifacts-${{ runner.os }}-${{ hashFiles('package-lock.json') }} | ||
| restore-keys: vmui-artifacts-${{ runner.os }}- | ||
| path: app/vmui/packages/vmui/node_modules | ||
| key: vmui-deps-${{ runner.os }}-${{ hashFiles('app/vmui/packages/vmui/package-lock.json') }} |
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.
The key field change looks better than the previous one, you may also want to account for the Dockerfile-build (because node_modules are produced inside a Docker image and depnd on the base image?)
key: vmui-deps-${{ runner.os }}-${{ hashFiles('app/vmui/packages/vmui/package-lock.json', 'app/vmui/Dockerfile-build') }}| vmui-builder-image -c "npm install && $(NPM_COMMAND)" | ||
| vmui-builder-image -c "npm ci && $(NPM_COMMAND)" | ||
|
|
||
| vmui-install: | ||
| NPM_COMMAND="true" $(MAKE) vmui-run-npm-command |
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.
I have two concerns...
What's the goal of these commands? running them manually or improving CI speed? If it's the latter, then npm i still runs 3x for lin, test, typecheck (not too long, but not too short either, and it still takes time), right? You might consider adding a VMUI_SKIP_INSTALL env var if these commands are also used manually.
If you change the workflow to run npm i only when needed (after resolving the above), then it's better to use npm ci (since it's faster and more secure). So the usual practice is:
- cache hit -> skip install (don't run
npm iornpm ciat all) - cache miss -> run
npm cionce -> cache node_modules
Describe Your Changes
Update GitHub Actions for vmui workflows: optimize caching and dependency installation process
Checklist
The following checks are mandatory: