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

meta: Fix yarn caching in github actions #3526

Merged
merged 5 commits into from
Mar 3, 2022
Merged

meta: Fix yarn caching in github actions #3526

merged 5 commits into from
Mar 3, 2022

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Mar 2, 2022

I can see that the "install dependencies" (yarn) step on our github actions is taking the majority of the time.
I believe that our caching logic is not valid, and that this may be the reason why.
I believe it should be like this: https://github.com/actions/cache/blob/main/examples.md#node---yarn-2

However the setup-node action also uses the cache action and hopefully will handle this correctly when specifying "yarn".

@Murderlon Murderlon requested a review from aduh95 March 2, 2022 13:47
@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

seems like it didn't help much

run1:

Screenshot 2022-03-02 at 21 44 32

run2:

Screenshot 2022-03-02 at 21 56 27

trying again with this: https://github.com/actions/cache/blob/main/examples.md#node---yarn-2

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

hmm. Still:

Post job cleanup.
/usr/bin/tar --posix --use-compress-program zstd -T0 -cf cache.tzst -P -C /home/runner/work/uppy/uppy --files-from manifest.txt
Cache Size: ~0 MB (22 B)
Cache saved successfully
Cache saved with key: Linux-yarn-4fe0077fcdbd2359492aec8320e9394f453875149230c817fb34a023075fe1e7

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

Aha!

  with:
    path: undefined
    key: Linux-yarn-4fe0077fcdbd2359492aec8320e9394f453875149230c817fb34a023075fe1e7
    restore-keys: Linux-yarn-
yarn config get cacheFolder
undefined
corepack yarn config get cacheFolder
/Users/mifi/Desktop/projects/transloadit/uppy/.yarn/cache

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

The Install Corepack if needed step should come before the corepack yarn config get cacheFolder call.

It looks like it does drastically improve the install time in most CIs but the Companion one. 🤔

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

The Install Corepack if needed step should come before the corepack yarn config get cacheFolder call.

I don't think so because it's using npm, not yarn (npm install -g corepack), so it's not cached.

It looks like it does drastically improve the install time in most CIs but the Companion one. 🤔

I think that it ran before the cache existed. Trying to re-run now

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

I don't think so because it's using npm, not yarn (npm install -g corepack), so it's not cached.

It's not a cache problem, if Corepack is not installed, you can't run corepack yarn config get cacheFolder – or you'll get command not found: corepack.

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

Ah yes, understood 😄☑️

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

I guess all the jobs that run on old node versions should have the Install Corepack if needed step too

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

and all should have --immutable

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

--immutable is the default anyway, there's little value to add it explicitly but feel free to do so

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

I guess all the jobs that run on old node versions should have the Install Corepack if needed step too

Isn't that the case already?

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

Isn't that the case already?

yes, seems to be the case already.

maybe we can remove --immutable from all jobs then

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

I re-ran the companion jobs (I think I had forgotten to re-trigger those the first time). now they are also much faster (2-3min)

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

7 occurrences of install --immutable
3 occurrences of install (without --immutable)

Yeah maybe it's worth adding it everywhere for consistency and to keep the diff minimal.

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

But then again in the future when someone adds a job and they forget to add —immutable someone could be having this discussion again. Eg why are some jobs with —immutable and some without

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

Right, but same applies if we remove the flag and someone opens a PR to add them back. Probably explicit is better than implicit, in case Yarn switches default in the future.

@mifi
Copy link
Contributor Author

mifi commented Mar 2, 2022

ok done!

@mifi mifi merged commit 32ea099 into main Mar 3, 2022
@mifi mifi deleted the fix-ci-yarn-cache branch March 3, 2022 06:26
Murderlon added a commit that referenced this pull request Mar 7, 2022
* main:
  provider-view: fix breadcrumbs (#3535)
  Update BACKLOG.md
  Update ru_RU.js (#3529)
  @uppy/companion: reorder reqToOptions (#3530)
  meta: Fix yarn caching in github actions (#3526)
  Release: [email protected] (#3525)
  fix unstable test
  replace debug
  Fix COMPANION_PATH (#3515)
  @uppy/angular: update ng version (#3503)
  Upload protocol "s3-multipart" does not use the chunkSize option (#3511)
  Add chunks back to prepareUploadParts, indexed by partNumber (#3520)
  website: Add “Stop the war” banner (#3518)
github-actions bot added a commit that referenced this pull request Mar 16, 2022
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   0.3.0 | @uppy/locales        |   2.0.8 |
| @uppy/aws-s3         |   2.0.8 | @uppy/provider-views |   2.0.8 |
| @uppy/companion      |   3.4.0 | @uppy/vue            |   0.4.6 |
| @uppy/compressor     |   0.2.3 | @uppy/robodog        |   2.4.0 |
| @uppy/core           |   2.1.6 | uppy                 |   2.8.0 |
| @uppy/drop-target    |   1.1.2 |                      |         |

- @uppy/aws-s3: fix wrong events being sent to companion (Mikael Finstad / #3576)
- @uppy/compressor: ignore remote files, calculate savings correctly (Artur Paikin / #3578)
- @uppy/companion: always log errors with stack trace (Mikael Finstad / #3573)
- meta: remove incorrect s3 documentation (Mikael Finstad / #3571)
- @uppy/companion: Companion refactor (Mikael Finstad / #3542)
- website: partial ooops (Artur Paikin)
- meta: run e2e workflow on the head branch instead of the base one (Antoine du Hamel / #3561)
- website: Use Plausible instead of Google Analytics (Artur Paikin / #3567)
- @uppy/vue: enforce use of file extension within the import path (Antoine du Hamel / #3560)
- @uppy/drop-target: ignore if dropped elements aren't files (Penar Musaraj / #3563)
- @uppy/core: Abstract restriction logic in a new Restricter class (Merlijn Vos / #3532)
- @uppy/companion: Fetch all Google Drive shared drives (Robert DiMartino / #3553)
- website: add blog post 2.4-2.7 (Artur Paikin / #3557)
- meta: fix e2e (Antoine du Hamel / #3562)
- meta: fix broken link (YukeshShr / #3559)
- meta: fix support of export declaration in source files (Antoine du Hamel / #3558)
- @uppy/companion: Order Google Drive results by folder to show all folders first (Robert DiMartino / #3546)
- meta: add corsOrigins to docs (Mikael Finstad / #3554)
- @uppy/audio: refactor to ESM (Antoine du Hamel / #3470)
- @uppy/locales: compressor cleanup (Antoine du Hamel / #3531)
- meta: fix CJS interop in Vite config (Antoine du Hamel / #3543)
- @uppy/companion: upgrade node-redis-pubsub (Mikael Finstad / #3541)
- @uppy/provider-views: provider-view: fix breadcrumbs (Artur Paikin / #3535)
- meta: Update BACKLOG.md (Artur Paikin)
- @uppy/locales: Update ru_RU.js (Sobakin Sviatoslav / #3529)
- @uppy/companion: reorder reqToOptions (Antoine du Hamel / #3530)
- meta: Fix yarn caching in github actions (Mikael Finstad / #3526)
vymao pushed a commit to vymao/uppy that referenced this pull request Mar 29, 2022
* main:
  provider-view: fix breadcrumbs (transloadit#3535)
  Update BACKLOG.md
  Update ru_RU.js (transloadit#3529)
  @uppy/companion: reorder reqToOptions (transloadit#3530)
  meta: Fix yarn caching in github actions (transloadit#3526)
  Release: [email protected] (transloadit#3525)
  fix unstable test
  replace debug
  Fix COMPANION_PATH (transloadit#3515)
  @uppy/angular: update ng version (transloadit#3503)
  Upload protocol "s3-multipart" does not use the chunkSize option (transloadit#3511)
  Add chunks back to prepareUploadParts, indexed by partNumber (transloadit#3520)
  website: Add “Stop the war” banner (transloadit#3518)
vymao pushed a commit to vymao/uppy that referenced this pull request Mar 29, 2022
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   0.3.0 | @uppy/locales        |   2.0.8 |
| @uppy/aws-s3         |   2.0.8 | @uppy/provider-views |   2.0.8 |
| @uppy/companion      |   3.4.0 | @uppy/vue            |   0.4.6 |
| @uppy/compressor     |   0.2.3 | @uppy/robodog        |   2.4.0 |
| @uppy/core           |   2.1.6 | uppy                 |   2.8.0 |
| @uppy/drop-target    |   1.1.2 |                      |         |

- @uppy/aws-s3: fix wrong events being sent to companion (Mikael Finstad / transloadit#3576)
- @uppy/compressor: ignore remote files, calculate savings correctly (Artur Paikin / transloadit#3578)
- @uppy/companion: always log errors with stack trace (Mikael Finstad / transloadit#3573)
- meta: remove incorrect s3 documentation (Mikael Finstad / transloadit#3571)
- @uppy/companion: Companion refactor (Mikael Finstad / transloadit#3542)
- website: partial ooops (Artur Paikin)
- meta: run e2e workflow on the head branch instead of the base one (Antoine du Hamel / transloadit#3561)
- website: Use Plausible instead of Google Analytics (Artur Paikin / transloadit#3567)
- @uppy/vue: enforce use of file extension within the import path (Antoine du Hamel / transloadit#3560)
- @uppy/drop-target: ignore if dropped elements aren't files (Penar Musaraj / transloadit#3563)
- @uppy/core: Abstract restriction logic in a new Restricter class (Merlijn Vos / transloadit#3532)
- @uppy/companion: Fetch all Google Drive shared drives (Robert DiMartino / transloadit#3553)
- website: add blog post 2.4-2.7 (Artur Paikin / transloadit#3557)
- meta: fix e2e (Antoine du Hamel / transloadit#3562)
- meta: fix broken link (YukeshShr / transloadit#3559)
- meta: fix support of export declaration in source files (Antoine du Hamel / transloadit#3558)
- @uppy/companion: Order Google Drive results by folder to show all folders first (Robert DiMartino / transloadit#3546)
- meta: add corsOrigins to docs (Mikael Finstad / transloadit#3554)
- @uppy/audio: refactor to ESM (Antoine du Hamel / transloadit#3470)
- @uppy/locales: compressor cleanup (Antoine du Hamel / transloadit#3531)
- meta: fix CJS interop in Vite config (Antoine du Hamel / transloadit#3543)
- @uppy/companion: upgrade node-redis-pubsub (Mikael Finstad / transloadit#3541)
- @uppy/provider-views: provider-view: fix breadcrumbs (Artur Paikin / transloadit#3535)
- meta: Update BACKLOG.md (Artur Paikin)
- @uppy/locales: Update ru_RU.js (Sobakin Sviatoslav / transloadit#3529)
- @uppy/companion: reorder reqToOptions (Antoine du Hamel / transloadit#3530)
- meta: Fix yarn caching in github actions (Mikael Finstad / transloadit#3526)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   0.3.0 | @uppy/locales        |   2.0.8 |
| @uppy/aws-s3         |   2.0.8 | @uppy/provider-views |   2.0.8 |
| @uppy/companion      |   3.4.0 | @uppy/vue            |   0.4.6 |
| @uppy/compressor     |   0.2.3 | @uppy/robodog        |   2.4.0 |
| @uppy/core           |   2.1.6 | uppy                 |   2.8.0 |
| @uppy/drop-target    |   1.1.2 |                      |         |

- @uppy/aws-s3: fix wrong events being sent to companion (Mikael Finstad / transloadit#3576)
- @uppy/compressor: ignore remote files, calculate savings correctly (Artur Paikin / transloadit#3578)
- @uppy/companion: always log errors with stack trace (Mikael Finstad / transloadit#3573)
- meta: remove incorrect s3 documentation (Mikael Finstad / transloadit#3571)
- @uppy/companion: Companion refactor (Mikael Finstad / transloadit#3542)
- website: partial ooops (Artur Paikin)
- meta: run e2e workflow on the head branch instead of the base one (Antoine du Hamel / transloadit#3561)
- website: Use Plausible instead of Google Analytics (Artur Paikin / transloadit#3567)
- @uppy/vue: enforce use of file extension within the import path (Antoine du Hamel / transloadit#3560)
- @uppy/drop-target: ignore if dropped elements aren't files (Penar Musaraj / transloadit#3563)
- @uppy/core: Abstract restriction logic in a new Restricter class (Merlijn Vos / transloadit#3532)
- @uppy/companion: Fetch all Google Drive shared drives (Robert DiMartino / transloadit#3553)
- website: add blog post 2.4-2.7 (Artur Paikin / transloadit#3557)
- meta: fix e2e (Antoine du Hamel / transloadit#3562)
- meta: fix broken link (YukeshShr / transloadit#3559)
- meta: fix support of export declaration in source files (Antoine du Hamel / transloadit#3558)
- @uppy/companion: Order Google Drive results by folder to show all folders first (Robert DiMartino / transloadit#3546)
- meta: add corsOrigins to docs (Mikael Finstad / transloadit#3554)
- @uppy/audio: refactor to ESM (Antoine du Hamel / transloadit#3470)
- @uppy/locales: compressor cleanup (Antoine du Hamel / transloadit#3531)
- meta: fix CJS interop in Vite config (Antoine du Hamel / transloadit#3543)
- @uppy/companion: upgrade node-redis-pubsub (Mikael Finstad / transloadit#3541)
- @uppy/provider-views: provider-view: fix breadcrumbs (Artur Paikin / transloadit#3535)
- meta: Update BACKLOG.md (Artur Paikin)
- @uppy/locales: Update ru_RU.js (Sobakin Sviatoslav / transloadit#3529)
- @uppy/companion: reorder reqToOptions (Antoine du Hamel / transloadit#3530)
- meta: Fix yarn caching in github actions (Mikael Finstad / transloadit#3526)
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.

3 participants