fix(pnpm): replace corepack use with manual parse #39844
Conversation
| 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.
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
| await writeLocalFile(packageFileName, existingPackageFileContent); | ||
|
|
||
| // Asumming that corepack only needs to modify the package.json file in the root folder | ||
| // Asumming that only the root package.json needs to modification |
There was a problem hiding this comment.
Ah, okay. Thought to remove the mention of corepack since corepack use is not run anymore.
Won't it be confusing to still have corepack in the comment? Corepack is not modifying any files anymore.
But I'll of course revert to the original comment if that's preferred
There was a problem hiding this comment.
It is still a corepack change, we can remove use command where necessary.
We should also link the original source and logic in the comment, btw so it's easier to reference in future.
There was a problem hiding this comment.
Yeah, point taken, we are basically mimicking corepack. Will revert and also add a link to corepackUtils.ts#L300
| ) { | ||
| const integrity = ( | ||
| await exec( | ||
| `npm view ${depName}@${newVersion} dist.integrity`, |
There was a problem hiding this comment.
this only return sha512, right?
There was a problem hiding this comment.
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.
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.
we can't assume that. we should throw an artifact error if there's something missing
There was a problem hiding this comment.
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
viceice
left a comment
There was a problem hiding this comment.
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.
| ) { | ||
| const integrity = ( | ||
| await exec( | ||
| `npm view ${depName}@${newVersion} dist.integrity`, |
There was a problem hiding this comment.
we should reuse the response from npm datasource instead of calling npm CLI here.
datasource can maybe return it as digest
3 participants
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, and this is a suggestion for a solution to fix it, based on the outline in #37772Context
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: