Creating a branch rule for non LTS staging branches

[marco-ippolito]

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

[targos]

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).

[richardlau]

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.

[aduh95]

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.

[marco-ippolito]

Updated the description to mention non LTS

[targos]

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.

[richardlau]

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).

[aduh95]

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?)

[targos]

When I had time to take care of staging branches, I used to push a few times per week.

[BethGriggs]

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.

[aduh95]

@targos do you mean LTS-staging branches, or also non-LTS ones?

[targos]

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)

[marco-ippolito]

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

[aduh95]

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.

[targos]

Now I understand why the checkbox says "above"

[targos]

If rulesets are not affected by this limitation, we should migrate asap

[targos]

What about that:

• Protect LTS staging branches
• Protect non-LTS staging branches

[marco-ippolito]

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

[aduh95]

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 Validated badge 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 :)

[marco-ippolito]

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 Validated badge 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 :)

why not both, I would be +1 on both To recap:

• have only backporters and releasers to be able to push in staging branches
• verify commit signatures

[marco-ippolito]

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