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

handling package-lock.json updates #1806

Closed
trentm opened this issue Nov 16, 2023 · 10 comments
Closed

handling package-lock.json updates #1806

trentm opened this issue Nov 16, 2023 · 10 comments

Comments

@trentm
Copy link
Contributor

trentm commented Nov 16, 2023

@pichlermarc I'm not sure what the right thing to do for package-lock.json updates is with the following.

#1771 went in, adding the package-lock.json file.

Then, the #1723 renovate PR was just merged. Looking at just a part of that change:

diff --git a/package-lock.json b/package-lock.json
index 70e9e172..4464f673 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -30606,23 +30607,23 @@
     "node_modules/systeminformation": {
-      "version": "5.21.15",
-      "resolved": "https://registry.npmjs.org/systeminformation/-/systeminformation-5.21.15.tgz",
-      "integrity": "sha512-vMLwsGgJZW6GvoBXVWNZuRQG0MPxlfQnIIIY9ZxoogWftUpJ9C33qD+32e1meFlXuWpN0moNApPFLpbsSi4OaQ==",
+      "version": "5.21.17",
+      "resolved": "https://registry.npmjs.org/systeminformation/-/systeminformation-5.21.17.tgz",
+      "integrity": "sha512-JZYRCbIjk3WuBV59A9/rTla2rROX+aAJ9uo2Z1dI+bjieORcukClN8rlM1zE9NYKpULSbaGc+KKct/870lO0DA==",

@@ -34355,21 +34356,21 @@
     "packages/opentelemetry-host-metrics": {
       "name": "@opentelemetry/host-metrics",
       "version": "0.33.2",
       "license": "Apache-2.0",
       "dependencies": {
         "@opentelemetry/sdk-metrics": "^1.8.0",
-        "systeminformation": "^5.0.0"
+        "systeminformation": "^5.21.17"
       },
       "devDependencies": {
...
  1. It updates the actually installed version of systeminformation from 5.21.15 to 5.21.17. That's well and good.
  2. The second hunk, however, looks potentially problematic. It updates the dependencies["systeminformation"] value for what I had understood was meant to be a cache of the value in "packages/opentelemetry-host-metrics/package.json". However the value in those two files are no longer the same thing. (My understanding comes from the somewhat loose docs of the package-lock.json format here -- https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#packages -- and from the following commands.)

If one gets the latest and tries to regenerate the package-lock.json file:

npm install --package-lock-only

The result is this diff that undoes all those second hunk changes:
trentm@81ea8df


I came across this same issue with sync'ing the "package-lock.json" file when trying to update my PR #1763 now that there is a package-lock.json file. The latest commit there is also "undoing" those renovate second hunk changes:
b0b74e0

Is it possible that renovate changes, like #1723, are not updating package-lock.json correctly?

@trentm
Copy link
Contributor Author

trentm commented Nov 16, 2023

It might be helpful to compare equivalent PRs from renovate and dependabot. One current example is for updating axios from 0.x to 1.x:

Notice that the renovate one is also "undoing" some of those second hunk changes, e.g.:
https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1790/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519R34357

(Aside: There is some serious undocumented voodoo in what dependabot is and is not doing to update package-lock.json files!)

@pichlermarc
Copy link
Member

pichlermarc commented Nov 17, 2023

Is it possible that renovate changes, like #1723, are not updating package-lock.json correctly?

Yes it also looks to me like the second part of changes should not have been part of the renovate PR (#1723). I was actually assuming that it was doing a lockFileMaintainance that was mis-titled as an update to the patch versions - I overlooked these changes when I was reviewing it.

It might be helpful to compare equivalent PRs from renovate and dependabot. One current example is for updating axios from 0.x to 1.x:

Notice that the renovate one is also "undoing" some of those second hunk changes, e.g.: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1790/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519R34357

(Aside: There is some serious undocumented voodoo in what dependabot is and is not doing to update package-lock.json files!)

Interstingly also in #1810, when it actually updates the patch versions, the dependencies in the package-lock.json revert back to what they should be (https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1810/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519R34364, the same as your draft PR #1807). I wonder if there's a bug in renvoate that caused that PR to be opened in the first place.

Another thing I've seen: when taking the package-lock.json from #1810 and comparing it with the package-lock.json from #1810 + npm install --package-lock-only, some devDependencies are reverted back to the actual caret version from the package.json. It looks to me like there's some deviation in how renovate handles it vs how npm handles it, where renovate replaces the caret version with the explicit version but with a caret prepended to it in the package-lock.json only. If my assumptions are correct, this should be easy to reproduce. I'll try that and then I'll come back to this issue with my findings.

Somewhat releated: another side effect from using the package-lock.json is that release-please does not seem to play well with it and that requires manual updates to the package-lock.json in the PR: #1798 (comment) - I have not figured out yet how to work around that.

@pichlermarc
Copy link
Member

I was able to reproduce the behavior that I saw in #1810 with a repo containing two packages and a top-level package.json that uses workspaces.

The two packges are set up as follows:

  • pkg-a depends on "@types/sinon": "10.0.11"
  • pkg-b depends on "@types/sinon": "^10.0.11"

Here's the PR renovate created on my reproducer repo: pichlermarc-sample-organization/repro-1806#6

This PR

  • updates the pinned version (we want that)
  • updates the resolved version in package-lock.json (we also want that)
  • does not update the caret version in the package's package.json (we also want that since it should resolve to the latest one)
  • does update the caret version in the package-lock.json (we don't want that as this is likely not correct - it does not reflect the actual dependency of that package in its package.json)

When switching to that branch, then running npm install --package-lock-only it does exhibit similar behavior to what I've seen in #1810 - I'll open an issue over at https://github.com/renovatebot/renovate and will link it here.

@pichlermarc
Copy link
Member

I created a discussion there as that is the recommended way to report bugs/problems, here's the link:
renovatebot/renovate#25847

@dyladan
Copy link
Member

dyladan commented Nov 17, 2023

@trentm it looks like in the PRs you linked dependabot did the right thing, unless I am misunderstanding. maybe the easiest solution is just to switch to dependabot?

@trentm
Copy link
Contributor Author

trentm commented Nov 17, 2023

@trentm it looks like in the PRs you linked dependabot did the right thing, unless I am misunderstanding. maybe the easiest solution is just to switch to dependabot?

@dyladan I think almost. Yes the dependabot PR (#1801) did the right thing for those "second hunk" changes, so that is better.

However what is going on here?
https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1801/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519R36314
https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1801/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519R45089

That sure seems like a dependabot bug. If you checkout the branch for that PR and run npm install --package-lock-only, I get this diff that reverts back that surprise

diff --git a/package-lock.json b/package-lock.json
index 23eebcbe..0709ee84 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -36311,7 +36311,7 @@
         "@opentelemetry/contrib-test-utils": "^0.34.3",
         "@opentelemetry/sdk-trace-base": "^1.8.0",
         "@types/mocha": "7.0.2",
-        "@types/mysql2": "git+ssh://[email protected]/types/mysql2.git#89378b2cb3974ea8cdd1d633b8f056e54e5d2384",
+        "@types/mysql2": "github:types/mysql2",
         "@types/node": "18.6.5",
         "@types/semver": "7.5.3",
         "mocha": "7.2.0",
@@ -45086,7 +45086,7 @@
         "@opentelemetry/semantic-conventions": "^1.0.0",
         "@opentelemetry/sql-common": "^0.40.0",
         "@types/mocha": "7.0.2",
-        "@types/mysql2": "git+ssh://[email protected]/types/mysql2.git#89378b2cb3974ea8cdd1d633b8f056e54e5d2384",
+        "@types/mysql2": "github:types/mysql2",
         "@types/node": "18.6.5",
         "@types/semver": "7.5.3",
         "mocha": "7.2.0",

That said, having a dependency on a github repo branch -- github:types/mysql2 -- should be avoided if possible, IMO. It makes locking impossible.

So, I'm inclined to say dependabot is doing a better job, but still has bugs we might have to workaround. For example, we might need/want to avoid dependencies other than version ranges.

workaround for "@types/mysql2": "github:types/mysql2",

There is this stale issue about publishing a version of @types/mysql2: types/mysql2#39
The last comment there suggests the types are part of mysql2 itself now, so @types/mysql2 should be unnecessary.
This includes the old [email protected] that "plugins/node/opentelemetry-instrumentation/mysql2/package.json" is using.

@trentm
Copy link
Contributor Author

trentm commented Nov 17, 2023

So, a possible path forward is:

  1. Merge chore: sync package-lock file to package.json files #1807 to sync package-lock.json.
  2. Merge a quick PR to stop using the @types/mysql2 dep: chore(instrumentation-mysql2): drop unneed @types/mysql2 dep #1812
  3. rebase the dependabot PR chore(deps): bump axios from 0.21.4 to 1.6.0 #1801 and check to see if its package-lock.json update is correct/sane. Merge that.
  4. Switch from renovate to dependabot. I have experience with dependabot, but none with renovate, so I'm not sure what functionality might be lost/missed.

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Nov 17, 2023
Types are included in 'mysql2' now (as of some 2.x release).
The github:types/mysql2 dependency is problematic for dependabot version updates.
See: open-telemetry#1806 (comment)
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Nov 17, 2023
Types are included in 'mysql2' now (as of some 2.x release).

The github:types/mysql2 dependency is problematic for dependabot version updates.
See: open-telemetry#1806 (comment)
pichlermarc added a commit that referenced this issue Nov 20, 2023
Types are included in 'mysql2' now (as of some 2.x release).

The github:types/mysql2 dependency is problematic for dependabot version updates.
See: #1806 (comment)

Co-authored-by: Marc Pichler <[email protected]>
jmcdo29 pushed a commit to jmcdo29/opentelemetry-js-contrib that referenced this issue Nov 21, 2023
…lemetry#1812)

Types are included in 'mysql2' now (as of some 2.x release).

The github:types/mysql2 dependency is problematic for dependabot version updates.
See: open-telemetry#1806 (comment)

Co-authored-by: Marc Pichler <[email protected]>
@trentm
Copy link
Contributor Author

trentm commented Nov 23, 2023

Has there been any more discussion about whether to continue to use renovate, or to switch to dependabot?

(I was looking at dependabot security alerts, and one of them would be fixed by upgrading mocha from v7 to v10. Originally that update PR (#1007) was closed because at the time it bumped the mocha min-supported Node.js beyond what this repo supported. However, since then this repo has bumped its min node version such that mocha v10 would be fine. I'm not sure whether to consider re-opening that renovate PR, or to look for an equivalent dependabot PR.)

@pichlermarc
Copy link
Member

pichlermarc commented Nov 27, 2023

Has there been any more discussion about whether to continue to use renovate, or to switch to dependabot?

(I was looking at dependabot security alerts, and one of them would be fixed by upgrading mocha from v7 to v10. Originally that update PR (#1007) was closed because at the time it bumped the mocha min-supported Node.js beyond what this repo supported. However, since then this repo has bumped its min node version such that mocha v10 would be fine. I'm not sure whether to consider re-opening that renovate PR, or to look for an equivalent dependabot PR.)

There has not been any discussion on it as far as I'm aware. I was hoping that the discussion I created in the renovate repo would yield some results, (last time I filed a bug with them they were very quick to respond and fix it - but the one I found back then was also higher priority), which would have enabled us to stick with renovate without changing anything.

Since it's been 10 days, I think not waiting anymore and switching to dependabot is reasonable, with the new grouping feature we should be able to replicate renovate bot's behavior. I'll create an issue (Created #1827)

(EDIT: yes we should update mocha if possible, let's try to use an equivalent dependabot PR)

@trentm
Copy link
Contributor Author

trentm commented Dec 6, 2023

Closing this, because I think it'll be resolved via #1827.

@trentm trentm closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants