Release
Release copied to clipboard
nodejs
Creating a branch rule for non LTS staging branches
Right now, everyone with push permission (collaborators) can push on non LTS staging branches. I believe we might want to change that to releasers only. Adding it to the agenda to seek consensus
This is not true. Only releasers and backporters can push to LTS staging branches. Additionally, collaborators can push to non-LTS staging branches (not sure why you mention moderation team, they don't have permission).
For reference https://github.com/nodejs/node/blob/f7e73cd1f2081d8b4a7e67b28ba90760a36ee8b3/doc/contributing/collaborator-guide.md?plain=1#L801-L802:
Only members of @nodejs/backporters should land commits onto LTS staging branches.
not sure why you mention moderation team, they don't have permission
Moderation team members are org Owner, and therefore have technically Write permission on the repo.
not sure why you mention moderation team, they don't have permission
Moderation team members are org Owner, and therefore have technically Write permission on the repo.
Yes, but it doesn't matter. We have setup branch protection rules that don't allow owners, admins, etc to bypass them.
We currently don't have a branch protection rule for v22.x-staging (or any other newly created branch that isn't caught by the existing rules).
Note that if we restrict it to Releasers, it will make onboarding harder. Maybe a better workflow would be to have some automation that sends a Slack notification everytime someone pushes to a staging branch, so if there are some suspicious activity, it can more easily be noticed (I assume pushing to staging branch is not something that happens often, is it?)
When I had time to take care of staging branches, I used to push a few times per week.
It's also common to rebase the staging branches frequently when triaging test results on the proposal (to remove problematic commits), or push additional commits on request.
I mean the staging branch for Current. I found it easier to continuously cherry-pick from main rather than doing everything just before the release proposal. For LTS, I also did it but in bigger batches (like dedicating one hour from time to time to cherry-picks / backport requests)
I think the staging branch (LTS and non LTS) should be treated equally as we are releasing them either way, that also makes it easier to write a rule that protects all branches ending with -staging
not sure why you mention moderation team, they don't have permission
Moderation team members are org Owner, and therefore have technically Write permission on the repo.
Yes, but it doesn't matter. We have setup branch protection rules that don't allow owners, admins, etc to bypass them.
not sure why you mention moderation team, they don't have permission
Moderation team members are org Owner, and therefore have technically Write permission on the repo.
Yes, but it doesn't matter. We have setup branch protection rules that don't allow owners, admins, etc to bypass them.
Regarding that, I'm afraid that's not true:
People and apps with admin permissions to a repository are always able to push to a protected branch or create a matching branch. Source: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#restrict-who-can-push-to-matching-branches
TSC voting members and Moderation team members, because they are org owners, can technically push to LTS staging branches, no matter they are protected branch.
To see what it would look like to push to a protected branch as an org owner, I activated that branch protection to node-auto-test, and try to push: I wasn't given a warning that I was doing an admin-only action.
What about that:
In today Release WG meeting https://github.com/nodejs/Release/issues/1011 we discussed a bit about this, and the idea of having just one rule to protect all staging branch sounds reasonable. This means only release will be able to push to staging branches. I'll open a PR to update the documentation and after that we can update the rule
To repeat what I said https://github.com/nodejs/Release/issues/1004#issuecomment-2083115849, I'm not sure adding a rule won't add frictions to onboarding new releasers. My recommendation would be to instead enforce commit signatures, because it:
- would guarantee that no one is pushing sneaky backports impersonating someone else.
- would not block collaborators who are interested in becoming releasers to prepare releases.
- would look nicer to have green
Validatedbadge on GitHub UI. - releasers have to setup a PGP key anyway to sign releases, it wouldn't require much config for them to also sign backport commits with that key (or they can use a SSH key if they have that setup).
When we discussed this on the call, @richardlau pointed out that it's also possible to add folks to backporters team, which non-collaborators who want to become releasers have to join anyway, so it wouldn't be inconsistent to require the same process for Collaborators, which is fair. I think I'm still in favor of requiring signature on backport commits no matter if we restrict who can push to the branch :)
To repeat what I said #1004 (comment), I'm not sure adding a rule won't add frictions to onboarding new releasers. My recommendation would be to instead enforce commit signatures, because it:
- would guarantee that no one is pushing sneaky backports impersonating someone else.
- would not block collaborators who are interested in becoming releasers to prepare releases.
- would look nicer to have green
Validatedbadge on GitHub UI.- releasers have to setup a PGP key anyway to sign releases, it wouldn't require much config for them to also sign backport commits with that key (or they can use a SSH key if they have that setup).
When we discussed this on the call, @richardlau pointed out that it's also possible to add folks to
backportersteam, which non-collaborators who want to become releasers have to join anyway, so it wouldn't be inconsistent to require the same process for Collaborators, which is fair. I think I'm still in favor of requiring signature on backport commits no matter if we restrict who can push to the branch :)
why not both, I would be +1 on both To recap:
- have only
backportersandreleasersto be able to push in staging branches - verify commit signatures
In todays meeting https://github.com/nodejs/Release/issues/1033 we had consensus that only backporters and releasers should be able to push in staging branches
Per decision I removed the access to collaborators to push on v23.x-staging, while other staging branches were already following the rule.
Maybe we can also remove some old rules? around v0 releases (like v0.8.16-release)
I can try to migrate the protection of very old branches to a single Ruleset.