-
Notifications
You must be signed in to change notification settings - Fork 135
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
Bot user for nodejs/corepack #660
Comments
/cc @nodejs/tsc |
I do not think this is a great idea. I propose that the config file include only the major version of each package manager. This will ensure that for a specific version of Node.js there won't be any breaking change. |
Limiting updates to major is a good idea, but it's tangential to having an automation - we'll still want to bump the minor / patch releases from the same major.
The config file contains exact versions so as one Corepack / Node version always provides the exact same default pnpm / Yarn versions, mirroring the way you currently distribute npm. |
No. I'm proposing to avoid the problem altogether and just specify the major version.
I would note that having a config file with the exact versions defeats one of the purposes of corepack. I'm proposing a change so we can actually avoid the problem completely. Given corepack download the package managers dynamically, we should not have users install versions with known vulnerabilities or even bugs if a fix is available. I do not think corepack should mirror what Node does with npm because we ship npm in source tree. This is a source of frequent reported vulnerabilities for Node.js (false positeves, usually RedDoS). |
Dynamic installs are a large topic, with some people seing them as a pro in terms of security (always up-to-date) while others see it as a con (faker.js situation). The current design avoids this problem by ensuring that the default versions are static (even if the binaries are lazy loaded), which matches the existing status quo. In my opinion, changing to dynamic versioning is a separate topic that's worthy to be debated on its own (and if everyone agrees to the change then so be it, I personally don't have strong feelings either way), but that shouldn't block improvements on the current design in the meantime. |
I'm +1 changing the default of Corepack to always be the most up-to-date semver-minor/semver-patch of all supported package managers. IMHO being up-to-date is much preferable than keeping an insecure version forever. I'm +1 having a bot that opens a PR to nodejs/corepack every time a new major version of npm or pnpm is released, and that opens a PR to nodejs/node every time a new version of Corepack is released. |
+1 to @mcollina's suggestion. I completely missed the fact that Corepack maintains a config file with specific versions in the original discussion. Even with automation to create the PRs to update Corepack, that means Corepack puts extra burden on the project to ship new releases any time one of the three package managers upgrade their defaults in the Corepack config. To add, I struggled to find the |
I agree with the sentiments in the last few comments. From my perspective the only reason some of us were willing to pull in corepack is because it avoided pulling the package managers into Node.js itself. If it's effectively the same thing and forces Node.js to do security updates when an update is needed in one of the package managers we just don't have the bandwidth to support that that (we already struggle with the CVEs etc. that we have to handle from including npm). |
On the TSC meeting it was decided to go on with adding an automation that opens a PR to nodejs/node every time a new version of Corepack is released. We have already a workflow in the core repo to update dependencies: https://github.com/nodejs/node/blob/HEAD/.github/workflows/tools.yml. |
Sure, will do 👍 |
Opened nodejs/node#42090 to auto-update Corepack in Node. However my initial points were also to allow automated releases in the Corepack repository; is it possible? Even if Corepack switches to dynamic versions, I'd still prefer to have some repository workflow for Corepack releases, and avoid having to release it by running commands from my own laptop. |
I don't think that makes sense. None of the packages are owned by the user, instead we add the nodejs-foundation as a backup user so that if the prior collaborators are no longer around we can add new ones. We have talked about creating an org, and in that case it might make more sense. Since you indicated this was optional, it is probably best to handle/discuss this aspect as a follow on task to avoid blocking the key request. I agree the concept of having a publishing flow that does not require an individual involvement makes sense. It is something we might like for node-addon-api as well (although not a big deal there as our releases are not too often).
My main question is with respect to 2fa/publishing. I think it's more complicated than just adding a token to the repo. @dominykas what would you suggestion on this front? |
According to our policy, we must add the nodejs-foundation user as admin for any project maintained package:
So if that's not the case yet, this is the first step. I'm almost sure we have used this same user to automate other packages that have automated release. If not, we really need to settle on this, adding user-created automated tokens is not the way to go for a project of our size. I'm +1 on automating the release flow for corepack by the way (I think we should do that for all our npm packages as part of our policy). |
I agree that user-created tokens should not be used for automation of publishing. I don't think we have automated publishing yet. There was also a suggestion that we should change the name of the nodejs-foundation user. This might be the right time to create a new user or org and start by using it for automation. |
+1. This would help unblocking nodejs/node-core-utils#511 |
What are the next steps regarding the publish bot user? |
I guess it would be to create an automation token for the nodejs-foundation user and add it as a secret to the nodejs/corepack repo (like #618)? |
@mhdawson can you take care of the token creation please?
@arcanis can you transfer the npm package ownership to |
@aduh95 done as requested above |
@aduh95 I invited |
@mhdawson Can you make sure that |
That's correct.
Thanks a lot! I assume the issue can now be closed :) |
I'd like to automate Corepack so that it'd automatically be updated with new package manager information, and republished. The setup I have in mind:
This would require the following:
Ref nodejs/corepack#90
The text was updated successfully, but these errors were encountered: