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: add caching. #605

Closed
wants to merge 1 commit into from
Closed

CI: add caching. #605

wants to merge 1 commit into from

Conversation

XhmikosR
Copy link
Contributor

If any of the following files changes, the cache will be invalidated:

  • package.json
  • package-lock.json
  • .github/workflows/ci.yml

This can probably be simplified but it should be a first start. Feel free to push any further changes to this branch.

@mikemimik mikemimik added this to the OSS - Sprint 1 milestone Dec 17, 2019
@XhmikosR XhmikosR marked this pull request as ready for review December 18, 2019 08:38
@XhmikosR XhmikosR requested a review from a team as a code owner December 18, 2019 08:38
@mikemimik mikemimik added Community Enhancement new feature or improvement semver:patch semver patch level for changes labels Dec 19, 2019
@mikemimik mikemimik self-assigned this Dec 19, 2019
If any of the following files changes, the cache will be invalidated:

* package.json
* package-lock.json
* .github/workflows/ci.yml
key: ${{ runner.os }}-node-v${{ matrix.node-version }}-${{ hashFiles('package.json') }}-${{ hashFiles('package-lock.json') }}-${{ hashFiles('.github/workflows/ci.yml') }}
restore-keys: |
${{ runner.OS }}-node-v${{ matrix.node-version }}-${{ hashFiles('package.json') }}-${{ hashFiles('package-lock.json') }}-${{ hashFiles('.github/workflows/ci.yml') }}
${{ runner.OS }}-node-v${{ matrix.node-version }}-
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will remove this so that when one of the above files change cache is invalidated. Better have this as a fallback mechanism. WDYT?

@mikemimik mikemimik added Release 6.x work is associated with a specific npm 6 release Wontfix this will not be worked on and removed Release 6.x work is associated with a specific npm 6 release labels Jan 21, 2020
@mikemimik
Copy link
Contributor

Hey @XhmikosR from our understanding of the caching action we're not sure we would see any benefits.

The rational being, with the key combination of {os}-{node-version}-{package.json} we never run npm install more than once on the same job in an action. This combination would generate a unique key for each item in the matrix of action runs. If we simplified it by removing {os} then it would be stating that the same npm cache could be used for windows and linux, which isn't something we'd want. By removing {node-version} then we'd have the same cache across node versions, which isn't ideal either.

I think at the moment we'll forgo adding a cache to the CI specifically in npm/cli as there is only one job for each action in the matrix, and this one job only runs npm install once anyhow.

This is definitely an improvement we'll want to look at in the future if testing and coverage get a little more involved, and perhaps in some of the dependencies of the npm/cli project itself.

Thanks so much for the suggestion, we didn't know that there was a caching action available! 👍🏽

(I'm closing the comment, but not the conversation. Please reach out if you think we've made a mistake)

@mikemimik mikemimik closed this Jan 22, 2020
@XhmikosR
Copy link
Contributor Author

Just FYI this is almost the same logic Travis CI is using.

The benefits aren't huge, I agree. But this could tweaked further as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement semver:patch semver patch level for changes Wontfix this will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants