-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(pnpm): replace corepack use with manual parse #39844
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
base: main
Are you sure you want to change the base?
Conversation
lib/modules/manager/npm/artifacts.ts
Outdated
| const [algo, b64] = integrity.split('-', 2); | ||
| const hex = Buffer.from(b64, 'base64').toString('hex'); | ||
|
|
||
| const expectedLen = algo === 'sha512' ? 128 : algo === 'sha256' ? 64 : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically sha384 could also be a possible algo, so could either add a check for correct length for 384 as well, or change to only check lenght on sha512, which should be the most common algo.
lib/modules/manager/npm/artifacts.ts
Outdated
| ) { | ||
| const integrity = ( | ||
| await exec( | ||
| `npm view ${depName}@${newVersion} dist.integrity`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only return sha512, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK as long as using the official npm registry, only sha512 will be returned (from 2017 and onward)
However with other registries this might not be the case. Corepack's own readme mentions sha224 for yarn for example, and was the reason for assuming more than sha512.
But I guess for renovate can we assume to always use the official npm registry, and then expect only sha512?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess for renovate can we assume to always use the official npm registry, and then expect only sha512?
@viceice thoughts?? I don't think npm regsitry will always be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't assume that. we should throw an artifact error if there's something missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should reuse the response from npm datasource instead of calling npm CLI here.
datasource can maybe return it as digest
|
The PR title is fine |
viceice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment. we should return the integrity value via the getDigest function of the npm manager if it's not already available in the usual npm response.
lib/modules/manager/npm/artifacts.ts
Outdated
| ) { | ||
| const integrity = ( | ||
| await exec( | ||
| `npm view ${depName}@${newVersion} dist.integrity`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should reuse the response from npm datasource instead of calling npm CLI here.
datasource can maybe return it as digest
Added a couple of commits addressing this. I'm not sure if this was the way you intended, but let me know if it works or needs changing. |
| let shasum = isShasum(digest) ? digest : ''; | ||
|
|
||
| const distInfo = { integrity: '', shasum: '' }; | ||
| if (!integrity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should i keep the fallback on the CLI if not available in the datasource, or just remove?
| const res = await getDependency(this.http, registryUrl, packageName); | ||
| return res; | ||
| } | ||
| override async getDigest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a new funciton. The getReleases fn already gets this info (integrity & shasum) when fetching new releases for the package managers.
Can you confirm if this is still true? Cause it was the last time I checked IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RahulGautamSingh Yeah, I checked that first, and I didn't find anything in the ReleaseResult interface that I could use, since it is not a npm-specific type.
@viceice also mentioned the getDigest function, so seemed the better choice.
I might be wrong though, so I can happily rewrite if I can get a hold on how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not in the ReleaseResult interface as we didn't need to track those fields previously. But, I think if you check the response you might find them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Where would i need to look then?
Tried to mock the npm request in the test file, and log out everything I get from the response from getReleases, and this is all I get:
{
"homepage": "https://pnpm.io",
"releases": [
{
"version": "8.15.6",
"dependencies": {},
"devDependencies": {},
"attestation": false,
"releaseTimestamp": "2024-01-01T00:00:00.000Z",
"constraints": {
"node": [
">=18.12"
]
}
}
],
"tags": {
"latest": "8.15.6"
},
"registryUrl": "https://registry.npmjs.org",
"sourceUrl": "git+https://github.com/pnpm/pnpm.git",
"isPrivate": true
}
Changes
Switches from using
corepack use [email protected]to parse it manually the same way corepack does it themselves. Have been struggling with renovate not updating pnpm correctly.This solution is based on the outline in #37772
First tries to get the
integrityfromdatasourceviagetDigest, if unvavailable, trying from CLI withnpm view. If still failing, then get theshasumfromdatasourceand if that fails, get it from the CLI withnpm view(got it from the firstnpm viewcommand)Falling back on
shasummimicks how corepack handles it: fix: fallback to shasum when integrity is not defined#542The reason for the fallback is that some npm registries does not define an
integrityfield.Context
Please select one of the following:
use#37772AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
The public repository: