refactor(ci): fix fetch-depth and add some caching #5563
Conversation
|
✨ code-server docs for PR #5563 is ready! It will be updated on every commit.
|
|
Ah, right, we have |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5563 +/- ##
=======================================
Coverage 72.44% 72.44%
=======================================
Files 30 30
Lines 1673 1673
Branches 366 366
=======================================
Hits 1212 1212
Misses 398 398
Partials 63 63 Continue to review full report at Codecov.
|
jsjoeio
force-pushed
the
jsjoeio/cache-money
branch
from
e0e014f to
733dac4
Compare
jsjoeio
changed the title
| @@ -40,9 +37,14 @@ jobs: | |||
|
|
|||
| - name: Install helm | |||
| uses: azure/[email protected] | |||
| with: | |||
| token: ${{ secrets.GITHUB_TOKEN }} | |||
There was a problem hiding this comment.
Fixes warning from helm. Token needed for v3 and later.
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Install helm kubeval plugin | ||
| run: helm plugin install https://github.com/instrumenta/helm-kubeval |
There was a problem hiding this comment.
We need this for our lint.sh to actually lint the Helm chart.
| submodules: true | ||
|
|
||
| - name: Install quilt | ||
| run: sudo apt update && sudo apt install quilt | ||
| uses: awalsh128/cache-apt-pkgs-action@latest |
| - name: Fetch dependencies from cache | ||
| id: cache-node-modules | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: "**/node_modules" | ||
| key: yarn-build-${{ hashFiles('**/yarn.lock') }} | ||
| restore-keys: | | ||
| yarn-build- |
There was a problem hiding this comment.
It looks like we could add this block to the package-linux-cross step as well.
I think we might want to add platform keys so we cache separately for different platforms. The reason being is that I vaguely recall seeing native modules not rebuild if they already exist even if they are for the wrong platform.
So something like:
key: yarn-build-linux-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
yarn-build-linux-
Also the yarn release:standalone script runs npm install and it would be nice to cache those modules as well but we could handle that in another PR. But broadly speaking I think we could remove the standalone script and put those steps straight into the yaml instead that way we can gate the install behind the cache hit. I also think we could just do it straight in release instead of copying everything to a release-standalone directory first.
There was a problem hiding this comment.
Alternatively we could remove just the npm install line and put that in the yaml. Or make yarn release:standalone take an argument for whether to install.
There was a problem hiding this comment.
Good thinking! I'm going to add this to my todo list since I didn't get to it here. I'm thinking we do the approach you suggested and move the platform steps out of build.yaml so maybe I can tackle this with that!
| - name: Fetch dependencies from cache | ||
| id: cache-node-modules | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: "**/node_modules" | ||
| key: yarn-build-${{ hashFiles('**/yarn.lock') }} | ||
| restore-keys: | | ||
| yarn-build- |
There was a problem hiding this comment.
Same comment as previous except yarn-build-macos-.
This adds caching for the
node_modulesin platform steps,quiltand fixes thefetch-depthso checkouts are faster.Also renames
ci.yaml->build.yaml.Fixes N/A