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

Implementation of node's caching #272

Merged
merged 10 commits into from Jun 30, 2021
Merged

Implementation of node's caching #272

merged 10 commits into from Jun 30, 2021

Conversation

dmitry-shibanov
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov commented Jun 22, 2021

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
@maxim-lobanov maxim-lobanov mentioned this pull request Jun 23, 2021
8 tasks
@dmitry-shibanov dmitry-shibanov marked this pull request as ready for review June 23, 2021 15:35
README.md Show resolved Hide resolved
.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
README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
Copy link
Member

@bryanmacfarlane bryanmacfarlane left a comment

Choose a reason for hiding this comment

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

LGTM. good job!

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

* npm run build

* sync branches

* rebuild project

* fix test

Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com>
@merceyz
Copy link

merceyz commented Jul 4, 2021

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
Copy link

0-vortex commented Jul 5, 2021

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
Copy link
Contributor

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
Copy link

0-vortex commented Jul 5, 2021

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
Copy link
Contributor

@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
Copy link

0-vortex commented Jul 5, 2021

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.

miketheman added a commit to miketheman/warehouse that referenced this pull request Jun 12, 2022
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>
miketheman added a commit to miketheman/warehouse that referenced this pull request Jun 12, 2022
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>
miketheman added a commit to miketheman/warehouse that referenced this pull request Jun 15, 2022
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>
di pushed a commit to pypi/warehouse that referenced this pull request Jun 15, 2022
* 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>
deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
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>
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

10 participants