Skip to content
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

[CI] Refactor yarn-install composite action #9613

Merged
merged 23 commits into from
Jan 15, 2025

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Jan 14, 2025

Introduction

Unless I'm mistaken in the existing yarn-install composite action dependencies are cached twice.

Cached through setup-node

As we're providing yarn value to cache inputs.
Setup node will cache the global .yarn

    - name: Setup Node.js and get yarn cache
      uses: actions/setup-node@v3
      with:
        node-version: 18
        cache: yarn

Programmatic node_modules caching

We even manually still cache node_modules folder using:

    - name: Cache node modules
      id: node-modules-cache
      uses: actions/cache@v3
      with:
        path: |
          node_modules
          packages/*/node_modules
        key: root-node_modules-${{ hashFiles('yarn.lock') }}
        restore-keys: root-node_modules-

By the way it seems like that the yarn.lock file is useless as the restore-key pattern omits it
This could, unless I'm mistaken, leads to invalid cache restore

Runtime

The current infra results in the following install deps logs:

Prepare all required actions
Getting action download info
Download action repository 'actions/setup-node@v3' (SHA:1a444[2](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:2)cacd436585916779262731d5b162bc6ec7)
Download action repository 'actions/cache@v[3](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:3)' (SHA:f4b3439a656ba812b8cb417d2d49f9c810103092)
Run ./.github/workflows/actions/yarn-install
Run actions/setup-node@v3
Found in cache @ /opt/hostedtoolcache/node/18.20.5/x6[4](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:4)
Environment details
/usr/local/bin/yarn --version
4.4.0
/usr/local/bin/yarn config get cacheFolder
/home/runner/.yarn/berry/cache
/usr/local/bin/yarn config get enableGlobalCache
true
Cache Size: ~421 MB (441369819 B)
/usr/bin/tar -xf /home/runner/work/_temp/883eae34-9b21-4684-96[5](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:5)8-cdb937f62965/cache.tzst -P -C /home/runner/work/twenty/twenty --use-compress-program unzstd
Cache restored successfully
Cache restored from key: node-cache-Linux-yarn-a8c18b6600f1bc685e60192f39a0a0c5c51b918829090129d3787302789547fc
Run actions/cache@v3
Cache Size: ~506 MB (530305961 B)
/usr/bin/tar -xf /home/runner/work/_temp/becd3767-ac80-4ca9-8d[21](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:23)-93fa61e3070c/cache.tzst -P -C /home/runner/work/twenty/twenty --use-compress-program unzstd
Cache restored successfully
Cache restored from key: root-node_modules-a8c18b6600f1bc685e60192f39a0a0c5c51b918829090129d378730[27](https://github.com/twentyhq/twenty/actions/runs/12770942233/job/35596946506#step:6:30)89547fc

Where we can see in details that a cache is hit twice.

Cache Size: ~421 MB (441369819 B)
Cache Size: ~506 MB (530305961 B)

Suggestion

We should stick to only one deps caching mechanism.
Also caching the node_modules folder is not recommended.
That's why I would factorize our caching to the setup-node scope only.

The cache-primary-key crafting

Within the cache-primary-key we will inject the following metrics:

  • Node version, as we're caching node_modules directly in this way to avoid cache desync in case we upgrade node.
  • Hash of the lockfile, in this way if the lockfile comes to mutate we will re-install the deps from scratch, in this way no need to programmatically listen for changed-files on the package.json and the lockfile
  • Strict installation with check-cache and enableHardenedMode to true and obviously the --immutable flag ( note adding the --immutable-cache by principle even if on paper is overkill as a cache restoration and install should never occurs )

@prastoin prastoin force-pushed the refactor-composite-action-yarn-install branch from e48f54d to 2d19265 Compare January 15, 2025 11:02
@prastoin prastoin force-pushed the refactor-composite-action-yarn-install branch from e47644c to 58b792c Compare January 15, 2025 13:12
@prastoin prastoin force-pushed the refactor-composite-action-yarn-install branch from 58b792c to 0cb9c5b Compare January 15, 2025 13:13
@prastoin
Copy link
Contributor Author

TODO: Centralize PATH to cache within a multiline outputs of the globals step

@prastoin prastoin force-pushed the refactor-composite-action-yarn-install branch from ef36dd8 to adb7bab Compare January 15, 2025 14:17
@prastoin prastoin marked this pull request as ready for review January 15, 2025 14:18
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR refactors the yarn-install composite action to improve dependency caching efficiency by eliminating redundant caching mechanisms and implementing a more robust caching strategy.

  • Added cache key builder step in /.github/workflows/actions/yarn-install/action.yaml that includes node version and lockfile hash for better cache invalidation
  • Replaced actions/cache with separate actions/cache/restore and actions/cache/save actions for more granular control
  • Enabled hardened mode and immutable cache flags for stricter dependency management
  • Fixed critical issue where cache key mismatch between restore and save operations could cause cache inconsistencies

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Really interesting work! I don't understand everything but I wrote a small question.

@charlesBochet charlesBochet merged commit 7cb259d into main Jan 15, 2025
35 checks passed
@charlesBochet charlesBochet deleted the refactor-composite-action-yarn-install branch January 15, 2025 14:46
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.

3 participants