Skip to content
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

Creating a branch rule for non LTS staging branches #1004

Open
marco-ippolito opened this issue Apr 29, 2024 · 20 comments
Open

Creating a branch rule for non LTS staging branches #1004

marco-ippolito opened this issue Apr 29, 2024 · 20 comments

Comments

@marco-ippolito
Copy link
Member

marco-ippolito commented Apr 29, 2024

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
Copy link
Member

targos commented Apr 29, 2024

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
Copy link
Member

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.

@marco-ippolito marco-ippolito changed the title Creating a branch rule for staging branches Creating a branch rule for non LTS staging branches Apr 29, 2024
@aduh95
Copy link
Contributor

aduh95 commented Apr 29, 2024

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
Copy link
Member Author

Updated the description to mention non LTS

@targos
Copy link
Member

targos commented Apr 29, 2024

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
Copy link
Member

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
Copy link
Contributor

aduh95 commented Apr 29, 2024

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
Copy link
Member

targos commented Apr 29, 2024

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

@BethGriggs
Copy link
Member

BethGriggs commented Apr 29, 2024

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
Copy link
Contributor

aduh95 commented Apr 29, 2024

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

@targos
Copy link
Member

targos commented Apr 29, 2024

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Apr 29, 2024

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
Copy link
Member

targos commented Apr 30, 2024

Now I understand why the checkbox says "above"

CleanShot 2024-04-30 at 07 14 23@2x

@targos
Copy link
Member

targos commented Apr 30, 2024

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

@targos
Copy link
Member

targos commented Apr 30, 2024

What about that:

@marco-ippolito
Copy link
Member Author

In today Release WG meeting #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
Copy link
Contributor

aduh95 commented Jun 27, 2024

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

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jun 28, 2024

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
Copy link
Member Author

In todays meeting #1033 we had consensus that only backporters and releasers should be able to push in staging branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants