Skip to content

Conversation

@dyladan
Copy link
Member

@dyladan dyladan commented Feb 2, 2021

LOOKING FOR VOTES

Is this a good idea? Give this a 👍 or 👎

Problem Summary

After checking in lockfiles in #1829 there have been some concerns raised:

  • dependency bots don't like lock files or handle them in inconsistent manners
  • "number of files changed" in PR reviews has become mostly useless
  • regenerating package lock files with lerna has become quite cumbersome

Simply checking in the node_modules will bring back the old issues. Namely that the caching step took a very long time in the old CI because caching so many small and deeply nested files in the node_modules structure was a very expensive operation.

Proposed Solution

This PR attempts to bridge the gap between what we had before and what we had with lockfiles by caching the lockfiles using the github cache instead of caching the node modules. On a cache hit, npm ci is used to enable a drastically faster install process, otherwise we fall back on npm install.

It keeps the root package-lock.json file as this file is not touched by lerna and enables drastically faster local and ci builds.

TL;DR

  • Keep root level package-lock.json
  • Remove package-lock.json and ignore it again
  • Instead of caching node_modules, cache the package-lock.json files
  • On a cache hit, use lerna bootstrap --ci to speed up bootstrap

Cache Hit

image

Cache Miss

image

@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #1896 (3c3d3a8) into main (3b5892b) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1896      +/-   ##
==========================================
+ Coverage   92.72%   92.80%   +0.07%     
==========================================
  Files         174      167       -7     
  Lines        6038     5669     -369     
  Branches     1283     1196      -87     
==========================================
- Hits         5599     5261     -338     
+ Misses        439      408      -31     
Impacted Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 94.73% <0.00%> (-0.81%) ⬇️
...lemetry-exporter-collector/src/transformMetrics.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ages/opentelemetry-exporter-collector/src/types.ts
.../opentelemetry-exporter-collector/src/transform.ts

@vmarchaud
Copy link
Member

I wonder why we hit those issues when updating them. I personally use them (with yarn instead of npm) without any problem with different monorepos.
Anyway if we can gain the CI speed without causing all those updating issue, i'm totally fine.

@Flarna
Copy link
Member

Flarna commented Feb 3, 2021

I think the main issue is that you can't just update a single subproject because this would add the lerna managed dependencies into the lockfile.
And once you start with deleting files/folders it's easy to mix something up - in special as there are that a lot folders.
Not sure if npm has also some built in logic to decide when to do a "real" install, when to (re-)generated a lockfile,...

#1890 is for sure a special case as it touches more or less all package.json files and as a result a lot package-lock.json files are effected.
And there were some leftovers from lock file intro, e.g. the meta packages had such a file but shouldn't have one (as they have no foreign dependencies), some tests projects were still include in the lerna project,...

We could wait a while to see if this gets better.

@dyladan
Copy link
Member Author

dyladan commented Feb 3, 2021

I think the main issue is that you can't just update a single subproject because this would add the lerna managed dependencies into the lockfile.

Not sure what you mean here. Can you give an example?

And once you start with deleting files/folders it's easy to mix something up - in special as there are that a lot folders.
Not sure if npm has also some built in logic to decide when to do a "real" install, when to (re-)generated a lockfile,...

Lockfiles are not used at all for a customer install. The latest version of dependencies which match the package.json will be used (or version specified in the customer's lockfile).

We could wait a while to see if this gets better.

#1770 could also help quite a bit.

@Flarna
Copy link
Member

Flarna commented Feb 3, 2021

I think the main issue is that you can't just update a single subproject because this would add the lerna managed dependencies into the lockfile.

Not sure what you mean here. Can you give an example?

consider you change a dependency in core. Then delete node_modules folder + package-lock.json there and type npm install in core folder. This results in a wrong lock file. You have to do the npm install in root folder instead.

@dyladan
Copy link
Member Author

dyladan commented Feb 3, 2021

I think the main issue is that you can't just update a single subproject because this would add the lerna managed dependencies into the lockfile.

Not sure what you mean here. Can you give an example?

consider you change a dependency in core. Then delete node_modules folder + package-lock.json there and type npm install in core folder. This results in a wrong lock file. You have to do the npm install in root folder instead.

ah yes. this has always been an issue. in that case though a bootstrap should be fast as the other projects will be ignored

@obecny
Copy link
Member

obecny commented Feb 3, 2021

lets get this merge asap, there are more and more PRs that are modifying lock files and conflicts are inevitable.

@dyladan dyladan merged commit 7e423a9 into open-telemetry:main Feb 4, 2021
@dyladan dyladan deleted the cache-lockfiles branch February 4, 2021 21:28
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants