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

Do not ivalidate the cache entirely on lock file change #744

Merged
merged 3 commits into from Jun 27, 2023

Conversation

dsame
Copy link
Contributor

@dsame dsame commented Apr 24, 2023

Description:

Add a function cache-utils.ts:repoHasYarn3ManagedCache returns true if there's at least one project with yarn3 managed cache

Yarn3 managed cache enables safe use of the previous cache cache.restoreCache(cachePaths, primaryKey, [keyPrefix])
because obsolete dependences are removed by Yarn not letting cache to grow endlessly

Related issue:
#325

The PR initially contained Node version in the key but it was removed due to
#325 (comment)

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@dsame dsame requested a review from a team as a code owner April 24, 2023 19:35
@dsame dsame marked this pull request as draft April 25, 2023 06:37
Copy link
Contributor

@marko-zivic-93 marko-zivic-93 left a comment

Choose a reason for hiding this comment

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

From the code standpoint it looks ok to me. I am not sure about the semantics :)

src/cache-restore.ts Outdated Show resolved Hide resolved
@dsame dsame force-pushed the yarn-perfomance branch 2 times, most recently from 8e8ef76 to 408539a Compare May 22, 2023 20:22
@dsame dsame force-pushed the yarn-perfomance branch 6 times, most recently from 8d52008 to 70c9389 Compare June 22, 2023 08:47
@dsame dsame marked this pull request as ready for review June 22, 2023 08:55
Copy link
Contributor

@nikolai-laevskii nikolai-laevskii left a comment

Choose a reason for hiding this comment

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

Looks good to me

src/cache-utils.ts Outdated Show resolved Hide resolved
@nikolai-laevskii
Copy link
Contributor

I'll keep my green light. Couldn't find something out of order, still looks good to me. But don't forget to rename branch from yarn-perfomance to yarn-performance before merging it into actions

Copy link
Contributor

@marko-zivic-93 marko-zivic-93 left a comment

Choose a reason for hiding this comment

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

LGTM!

src/cache-utils.ts Show resolved Hide resolved
src/cache-utils.ts Outdated Show resolved Hide resolved
@dsame dsame force-pushed the yarn-perfomance branch 5 times, most recently from 5c6f97e to 22005da Compare June 26, 2023 08:11
@dsame dsame force-pushed the yarn-perfomance branch 2 times, most recently from 3a690c3 to 71e53d3 Compare June 26, 2023 10:07
src/cache-utils.ts Outdated Show resolved Hide resolved
@dsame dsame force-pushed the yarn-perfomance branch 10 times, most recently from b4911ac to d583854 Compare June 26, 2023 20:42
@belgattitude
Copy link

Hey thanks ! a small question

Does it disable the node_modules folder caching when yarn3 zip files are enabled ?

@dsame
Copy link
Contributor Author

dsame commented Jun 27, 2023

Does it disable the node_modules folder caching when yarn3 zip files are enabled ?

Hello @belgattitude, the PR does not change the caching strategy in part of node_modules folder, but yarn3 replaces node_modules with PnP that is supposed to be managed through VCS

@MaksimZhukov MaksimZhukov merged commit e33196f into actions:main Jun 27, 2023
157 checks passed
@arcanis
Copy link

arcanis commented Jul 16, 2023

Hey - I'm trying to understand this change. Does it mean that the best thing a Yarn project running with this action can do is to NOT use the global cache (enableGlobalCache: false) AND to also NOT check-in the .yarn/cache folder?

In Yarn v4 we're changing the default to make enableGlobalCache: true the default - would it make sense for us to keep it to false when the install is running on a GitHub action, and only there?

@arcanis
Copy link

arcanis commented Aug 6, 2023

Ping @dsame ? We'll soon release the 4.0 stable; do we need to automatically set enableGlobalCache: false on GH Actions?

@belgattitude
Copy link

Personally I wouldn't.

The enableGlobalCache new default won't be enough to get the benefits (action cache keys are part of the recipe too). I would let the setup responsability to setup/node or any tool/action/script that help setting up caches.

Applying different defaults depending on env might also create confusion.

I'd prefer a section in the yarn doc that focuses on a simple recipe for ci and caches (possibly with less emphasis on offline mode or a reference to it as an advanced use case.).

That would help people implementing cache strategies (azure, vercel... ) to identify quickly what variables are important, what they do and apply a strategy within their "platform" possibilities.

Congrats for Yarn 4.0

@MaksimZhukov
Copy link
Contributor

Hello @arcanis!

I'm trying to understand this change. Does it mean that the best thing a Yarn project running with this action can do is to NOT use the global cache (enableGlobalCache: false) AND to also NOT check-in the .yarn/cache folder?

Yes, if the user uses the local cache and .yarn/cache folder is not in the repository, the action will automatically try to restore previous cache even if it is outdated in order to speed up the build.

In Yarn v4 we're changing the default to make enableGlobalCache: true the default - would it make sense for us to keep it to false when the install is running on a GitHub action, and only there?

Thank you for contacting us and letting us know, we really appreciate it! In our opinion, it is not necessary to leave enableGlobalCache set to false for GitHub Actions, because this inconsistency can lead to confusion. Users can change this option if they want to use the local cache. We will reflect this behaviour in the action documentation to make it clear.

@belgattitude
Copy link

@arcanis for context, I realized there was a thread about the new setup node cache. It starts from there #325 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants