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

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 Jun 23, 2021
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

LGTM. good job!

maxim-lobanov and others added 5 commits Jun 29, 2021
* regenerate license

* npm run build

* sync branches

* rebuild project

* fix test

Co-authored-by: Dmitry Shibanov <dmitry-shibanov@github.com>
@maxim-lobanov maxim-lobanov merged commit e215578 into actions:main Jun 30, 2021
85 checks passed
@merceyz
Copy link

@merceyz 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 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

@jacobwgillespie jacobwgillespie 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.

@0-vortex
Copy link

@0-vortex 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
Collaborator

@maxim-lobanov maxim-lobanov commented Jul 5, 2021

@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 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.

HitlerLive
Copy link

HitlerLive commented on d36a331 Sep 21, 2021

💪

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

Successfully merging this pull request may close these issues.

None yet