Implementation of node's caching #272

dmitry-shibanov · 2021-06-22T15:45:48Z

In scope of this pull request we add possibility to cache node dependencies. Add cache input parameter to actions/setup-node. For now, input will accept the following values:

npm - enable caching for npm dependencies
yarn - enable caching for yarn dependencies

Related issue: #271

Description

Action will try to search package-lock.json or yarn.lock (npm 7.x supports yarn.lock files) files in the repository root and throw error if no one is found. The hash of found file will be used as cache key (the same approach like actions/cache recommends). The following key cache will be used ${{ runner.os }}-npm-${{ hashFiles('') }}
Action will cache global cache:

Npm (retrieved via npm config get cache)
Yarn 1 (retrieved via yarn cache dir)
Yarn 2 (retrieved via yarn config get cacheFolder)

Example of yml

For npm

 uses: actions/setup-node@v2
  with:
    node-version: '14'
    cache: 'npm'

For yarn:

 uses: actions/setup-node@v2
  with:
    node-version: '14'
    cache: 'yarn'

ADR: #266
Note:

It's not breaking change because it requires additional field and nothing changes by default.
License tests are falling. But we have changes to fix it. We've decided to stay with implementation, because approximately 50 licenses were changed. When changes are reviewed, we will add licenses to the existing pull request.

What's done:

- Implementation of node's caching
- Adding tests for node's caching
- Adding documentation
- Update licenses

* first iteration for implementation of caching * add logs * add debug line * fix build command * fix path * add possible post-if * remove braces * test new action post-if variant * work on built-in caching * remove post-if * pass version * work on yarn support * fix return value * change names and remove logs * worked on resolving comments * check post-if for null * add success() condition * remove primary key field * work on resolving comments * remove logs * resolving comments * resolving comments * resolving comments * resolving comments * fix getpackageManagerVersion * run clean for unstaged changes * fix falling version tests * work on resolving comments * resolving comments * fix comment * resolve comments * Add tests to cover node's caching (#3) * add tests to cover node's caching * work on fixing tests * fix e2e tests * rebuild and fix test * fixing tests * change name of describes, it and fix test * add names for jobs * fix issue

commit 446068a Author: Alena Sviridenko <alenasviridenko@github.com> Date: Tue Jun 22 17:51:35 2021 +0300 updated headers commit d7e254e Author: Alena Sviridenko <alenasviridenko@github.com> Date: Thu Jun 17 17:35:34 2021 +0300 updated links commit ffd9956 Author: AlyonaSviridenko <alenasviridenko@github.com> Date: Thu Jun 17 17:33:41 2021 +0300 Added advanced usage commit 1e068f0 Author: AlyonaSviridenko <alenasviridenko@github.com> Date: Thu Jun 17 15:07:42 2021 +0300 Updated readme with caching commit 7528c33 Author: Maxim Lobanov <v-malob@microsoft.com> Date: Wed Jun 16 14:43:46 2021 +0300 Update versions.yml

README.md

.github/workflows/e2e-cache.yml

.github/workflows/versions.yml

src/cache-restore.ts

src/cache-save.ts

README.md

action.yml

bryanmacfarlane

LGTM. good job!

* regenerate license * npm run build * sync branches * rebuild project * fix test Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com>

merceyz · 2021-07-04T10:47:58Z

The following key cache will be used ${{ runner.os }}-npm-${{ hashFiles('') }}

A bit late to the party but the cache for yarn@>=2 is identical across all operating systems so that doesn't need to be part of the key

0-vortex · 2021-07-05T10:27:13Z

Some high entropy questions:

any particular reason for the decision that shrinkwrap is not a lockfile
why is this action called setup-node if it only fully supports yarn
why is it tagged v2 if it's a breaking change, when v2 even had a v2-beta

This PR also says it's only supporting files in the root of the module but that just seems like cutting corners on customising the cache key, providing opinionated defaults without a way to configure them is simply pushing this PR's opinions on people or forcing them to compete in implementation.

Currently, the only way forward is to completely change the way we use npm (slash migrate to yarn) or switch all the workflows to v2-beta in order to avoid various tools picking the change up and suggesting action update.

jacobwgillespie · 2021-07-05T10:37:22Z

why is it tagged v2 if it's a breaking change

@0-vortex see above, specifically this line:

It's not breaking change because it requires additional field and nothing changes by default.

This PR adds a new feature where if you were previously using actions/setup-node and actions/cache, you might be able to remove actions/cache by adding the new cache key here. But you don't have to, you can continue using your current config, this PR doesn't change existing behavior.

0-vortex · 2021-07-05T11:08:58Z

why is it tagged v2 if it's a breaking change

@0-vortex see above, specifically this line:

It's not breaking change because it requires additional field and nothing changes by default.

This PR adds a new feature where if you were previously using actions/setup-node and actions/cache, you might be able to remove actions/cache by adding the new cache key here. But you don't have to, you can continue using your current config, this PR doesn't change existing behavior.

Thank you for the reply but it is not a clarification, sorry if the comment felt rushed, have spent a decent amount of hours toying with this implementation without getting positive results. As a precaution I have read all the comment history and reviewed the code changes twice before posting the questions.

Some references:

Motivation here is to fix/implement cache for a number of failing repositories while promoting best practices.

maxim-lobanov · 2021-07-05T12:02:32Z

@0-vortex , We don't pursue the goal to provide wide customization of caching in scope of actions/setup-node action. The purpose of this integration is covering ~90% of basic use-cases. If user needs flexible customization, they should continue to use actions/cache.

Currently, this integration covers basic use-cases for both npm and yarn:

As @jacobwgillespie fairly mentioned above,

This PR adds a new feature where if you were previously using actions/setup-node and actions/cache, you might be able to remove actions/cache by adding the new cache key here. But you don't have to, you can continue using your current config, this PR doesn't change existing behavior.

New functionality is added under additional flag cache. if you don't add new flag - everything will work as previously. So this PR is not breaking change.

Your suggestion about shrinkwrap file is fair enough and it would be helpful if you create feature request issue for it?
Until it is implemented, I suggest continue using actions/cache that provides more customized configuration.

0-vortex · 2021-07-05T12:17:55Z

Can you share where you get those stats from? Shrinkwrap is a standard production practice, don't understand how you can call it a suggestion.

The `actions/setup/node@v2` introduced support for configuring cache for npm. Since we use defaults, no changes are necessary other than setting the installer type. Refs: actions/setup-node#272 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

* chore: replace custom cache step with config The `actions/setup-python@v2` and higher included a built-in cache restore and post-run save step, meaning we can now retire our custom steps for the configuration we want and gain some speed here. Refs: actions/setup-python#266 Refs: https://github.com/actions/setup-python#caching-packages-dependencies Signed-off-by: Mike Fiedler <miketheman@gmail.com> * chore: replace custom cache step with config The `actions/setup/node@v2` introduced support for configuring cache for npm. Since we use defaults, no changes are necessary other than setting the installer type. Refs: actions/setup-node#272 Signed-off-by: Mike Fiedler <miketheman@gmail.com>

Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 4.29.1 to 4.29.3. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.29.3/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

dmitry-shibanov added 4 commits June 16, 2021 09:52

remove eol in gitattributes

05f3e9f

remove workaround

1088324

maxim-lobanov mentioned this pull request Jun 23, 2021

Support LTS aliases #270

Merged

8 tasks

dmitry-shibanov marked this pull request as ready for review June 23, 2021 15:35

AlenaSviridenko approved these changes Jun 23, 2021

View reviewed changes

bbugh reviewed Jun 24, 2021

View reviewed changes

README.md Show resolved Hide resolved

maxim-lobanov approved these changes Jun 24, 2021

View reviewed changes

.github/workflows/e2e-cache.yml Outdated Show resolved Hide resolved

.github/workflows/versions.yml Outdated Show resolved Hide resolved

src/cache-restore.ts Outdated Show resolved Hide resolved

src/cache-save.ts Outdated Show resolved Hide resolved

al-cheb approved these changes Jun 24, 2021

View reviewed changes

konradpabjan approved these changes Jun 24, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

action.yml Outdated Show resolved Hide resolved

work on resolving comments

d36a331

bryanmacfarlane approved these changes Jun 28, 2021

View reviewed changes

Maxim Lobanov and others added 5 commits June 29, 2021 13:34

regenerate license (#7)

282049a

* regenerate license * npm run build * sync branches * rebuild project * fix test Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com>

fix for documentation

7292279

Merge branch 'main' into add-caching-for-node-dependencies

55e1049

rebuild project

b259ba4

update dependencies

cfe70f9

maxim-lobanov merged commit e215578 into actions:main Jun 30, 2021

bbugh mentioned this pull request Jun 30, 2021

Support cache dependencies for cases where package lock files are located in subfolders #275

Closed

jacobwgillespie mentioned this pull request Jun 30, 2021

Add pnpm caching support #278

Merged

desrosj mentioned this pull request Jul 5, 2021

Use NPM caching built into action/setup-node WordPress/gutenberg#33190

Merged

7 tasks

spacedmonkey mentioned this pull request Jul 5, 2021

Workflows: Use built-in cache support in actions/setup-node GoogleForCreators/web-stories-wp#8189

Merged

9 tasks

svanherk mentioned this pull request Jul 20, 2021

ci: using built-in caching from setup-node BrightspaceUI/core#1501

Merged

web-padawan mentioned this pull request Jul 27, 2021

chore: use actions/setup-node built-in cache vaadin/web-components#2273

Merged

RusherRG mentioned this pull request Aug 1, 2021

[Fixes #271] feat: add ci for react Auto-DL/Auto-DL#272

Merged

12 tasks

merceyz mentioned this pull request Sep 10, 2021

Performance opportunity for yarn 3 cache #325

Closed

miketheman mentioned this pull request Jun 12, 2022

chore: replace custom cache step with config pypi/warehouse#11572

Merged

Thompson1985 approved these changes Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of node's caching #272

Implementation of node's caching #272

dmitry-shibanov commented Jun 22, 2021 •

edited

Loading

bryanmacfarlane left a comment

merceyz commented Jul 4, 2021

0-vortex commented Jul 5, 2021

jacobwgillespie commented Jul 5, 2021

0-vortex commented Jul 5, 2021

maxim-lobanov commented Jul 5, 2021

0-vortex commented Jul 5, 2021

Implementation of node's caching #272

Implementation of node's caching #272

Conversation

dmitry-shibanov commented Jun 22, 2021 • edited Loading

Description

Example of yml

What's done:

bryanmacfarlane left a comment

Choose a reason for hiding this comment

merceyz commented Jul 4, 2021

0-vortex commented Jul 5, 2021

jacobwgillespie commented Jul 5, 2021

0-vortex commented Jul 5, 2021

maxim-lobanov commented Jul 5, 2021

0-vortex commented Jul 5, 2021

dmitry-shibanov commented Jun 22, 2021 •

edited

Loading