fix: add corepack to docker image#8386
fix: add corepack to docker image#8386dcyrille18 wants to merge 1 commit intodependency-check:mainfrom
Conversation
Dockerfile
Outdated
| curl -Ls https://yarnpkg.com/latest.tar.gz | tar -xz --strip-components=1 --directory /opt/yarn && \ | ||
| ln -s /opt/yarn/bin/yarn /usr/bin/yarn && \ | ||
| npm install -g pnpm && \ | ||
| npm install -g pnpm corepack && \ |
There was a problem hiding this comment.
It's likely not correct to leave pnpm there.if installing corepack.
There was a problem hiding this comment.
Depending on the user project configuration, the analyzers could use pnpm or corepack, no ?
There was a problem hiding this comment.
corepack is a way to install/run pnpm (and yarn) - optionally with locked versions. When you install both you likely make a mess of the PATH.
We shouldn't have multiple ways to do the same thing in a container image, and in my view nor should it be a bag of different tools/approaches that various people like to bootstrap their tools.
We should pick one approach that we think is best for most users.
There was a problem hiding this comment.
So perhaps we can just install directly the latest NodeJS LTS and let corepack configure theirs environments (yarn, npn, pnpm) ?
There was a problem hiding this comment.
Maybe - npm -> corepack -> yarn | pnpm
No choice if we want to use corepack , since corepack isn't bundled with Node on Alpine, as you showed earlier.
But we might want to pre-cache a version for those not using package manager directive so it's not dynamically downloading in every single case (which is not easy to decide).
And need to actually research and see where pnpm and yarn are going now that corepack is no longer distributed with NodeJS. If they are ditching corepack and won't recommend use with it there's no point adding it here and we'll just be storing up problems for later.
There was a problem hiding this comment.
It seems corepack is no more distributed by the NodeJS from >25 but already recommended to manage properly the packages managers.
This description looks LLM generated, because it mKes.assertions about the change that are already already true before the change, but i don't believe this particular point is actually correct. Did you test this before/after? When you install the package globally, from memory im pretty sure the shims are all crested per the package.json and on the path. IIRC I'll have another look later, but this looks messy to me, given yarn and pnpm are still being manually installed. Should probably use only one 9r the other install approach - not both. |
Well seen, I ask to copilot to help me to write this change description because we turn around since many hours on how to get owasp working with our NodeJS project using yarn berry and corepack.
Yes, of course, I tested and I thought it works but after more tests, it seems not totally in this first try. Let me add a short description on what we have before: And after the first MR try (not fully automated for CI/CD): After further test, it seems that adding the environnement variable COREPACK_ENABLE_DOWNLOAD_PROMPT=false make the yarn version download totally transparent: |
eb59de3 to
9fcbd7d
Compare
|
In my opinion if we're going to do this we should probably switch the whole image to use corepack and only corepack. I'm not sure if there is value in both, and probably only downsides. I guess corepack isn't there currently because the image is installing npm directly; not node. |
9fcbd7d to
dd19330
Compare
|
Update done as requested with prepached images for yarn (v1) and pnpm (latest): Current: After MR: This build will now allow users to install custom package managers if needed. |
dd19330 to
0110cd0
Compare
marcelstoer
left a comment
There was a problem hiding this comment.
I do not support this proposal. Specific extensions should be handled in downstream images that extend from ours.
Not sure how familiar you are with the node/npm ecosystem but it’s a bit more complex than “specific extensions” in my view. We already pre-package node, npm, yarn and pnpm using disparate approaches. The ODC code for these analyzers is already “corepack compatible/aware”. For many years (prior to Node 25) corepack was pre-distributed with all Node versions (like with npm) and for all of these alternate package managers was the recommended approach to either bootstrap or install them, and provide a mechanism to “lock” ones version manager for reproducibility. Corepack shimmed the entry point scripts and would look at the In other words, prior to Node 25, one could consider corepack being installed and enabled to be a sensible default for a “Swiss army knife” image like this which already pre-packages all sorts of tools. For some reason node voted not to package it anymore, which adds friction for every single alternate package manager. It’s worth noting that this change will not graduate to LTS until April/May 2027 so for now, corepack is still “LTS”ish. According to OP, there appears to be a gap in the ODC image at the moment that causes yarn and pnpm to not work in a corepack aware way, and our image was possibly not “caught up” when the Yarn 2+ support was finally fixed in ODC itself. I think we need to confirm what these problems are. This is important now because yarn/pnpm are directly invoked by ODC and failure to use the correct version implied by package.json will cause scanning failures. It’s likely only an accidental side effect of Alpine’s non-standard packaging process for node/npm that corepack has not been included earlier. Most distros follow nodes official packaging and include it. Some other distros also split corepack from node, but they include a proper package for it, so you’re not installing into the npm global space (Arch, Wolfi, newer Debian). For some reason Alpine has no distinct corepack package, however provides corepack in a compliant way with the Now, to summarise my earlier messages, the messy bit is determining how to move forward and requires some research into whether pnpm, yarn and others will continue to recommend corepack. It doesn’t make sense for us to add it if everyone is moving away. In retrospect it’d probably have been better to open an issue to discuss the problem before submitting a solution, but it’s not the end of the world, as there’s not too much effort here I think. In my personal view, if corepack still has a role, rather than using npm global space, I would suggest
But I think we need to figure out exactly which combinations are not working right now with the pre-installed versions and whether corepack still has a role - before we decide to do anything. |
|
👍 Thanks for the effort that went into this write-up! Much appreciated. |
|
Yeah, sorry for the novel. I am interested in finding a consistent way forward as I recently ended up using corepack in the build to ensure we can run tests reliably since right now most of such integration/functional tests are coded to “don’t run test if yarn/pnpm etc not found” which is basically useless in ensuring we don’t break functionality since tests won’t even run if we set up the build incorrectly. I used corepack for simplicity so I could start ensuring these tests actually have a version of yarn to run, but will change the approach for consistency with whatever we choose for docker, if it makes sense to do so. |
0110cd0 to
f91f33f
Compare
Description of Change
This merge request adds Corepack to the base OWASP Dependency-Check Docker image.
The goal is to provide native support for package managers such as Yarn and pnpm without requiring downstream images or CI pipelines to manually install Corepack.
This change enables:
No breaking changes are introduced:
If Node.js tooling is not needed, the image behaves exactly as before.
Corepack remains disabled until explicitly invoked (e.g., corepack enable), following Node.js conventions.