Require pull request reviews before merging to release branches

[targos]

Yesterday, a commit was pushed by mistake to the release branch v11.x (nodejs/node#24389 (comment)).
To prevent this from happening again in the future, I would like to discuss the possibility of requiring pull request reviews before merging to release branches.
I think we don't need to require more than one review. To me, this seems like a good thing to do to prevent mistakes but also to have more collaborators involved in the release process. I always ping @nodejs/collaborators in release PRs but often get no comments or suggestions regarding the changelog.

Additionally, since all releasers must have a GPG key to sign the release tag, we could also require signed commits.

[MylesBorins]

We generally don't have review on the "working on x.y.z" commit, how would we rectify that?

…

On Tue, Nov 20, 2018, 4:32 AM Michaël Zasso ***@***.*** wrote: Yesterday, a commit was pushed by mistake to the release branch v11.x (nodejs/node#24389 (comment) <nodejs/node#24389 (comment)>). To prevent this from happening again in the future, I would like to discuss the possibility of requiring pull request reviews before merging to release branches. I think we don't need to require more than one review. To me, this seems like a good thing to do to prevent mistakes but also to have more collaborators involved in the release process. I always ping @nodejs/collaborators in release PRs but often get no comments or suggestions regarding the changelog. Additionally, since all releasers must have a GPG key to sign the release tag, we could also require signed commits. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#392>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV7H3_Nce99sVevHhDf1146Hv3LHuks5uw8wVgaJpZM4Yqs8-> .

[targos]

Ah, right... I think it's possible to change the process to push first to the PR and then to the branch.

[richardlau]

How does this proposal prevent nodejs/node#24389 (comment)? That PR targetted master and was reviewed and correctly landed on master but was then landed directly on v11.x.

Release branches are already protected, so it's only a small number of users that can push to those branches.

[targos]

This prevents it because to land something on v11.x you would have to open a new pull request (cherry-picks are not allowed automatically). I agree that only a small number of users can push to those branches but the mistake could happen again and it's not ideal to have to unprotect all release branches to fix it.

I think requiring reviews is a good thing independently of this particular case: involve more collaborators (even if it's just one, it would be at least 2x more than now) and it's also easy to push to the wrong branch in the terminal while preparing a release.

[Fishrock123]

oh shit, I forgot to merge to staging rather than actual.

tells you how absurdly complex our release process is 🙄

[rvagg]

Well, in practical terms, releasers own those branches, so you could just clean them up however you like. If someone accidentally lands something on a .x branch then you could just force push it out to clean it up. Since the number of people managing these branches is small I don't think that's going to mean much toe stepping. I wouldn't object if the person doing the next Current took the initiative to clean out anything mistakenly on v11.x, that just seems like a reasonable part of the process, no?

[MylesBorins]

I think this is part of what our contract is on the .x branches… up until now we’ve been treating them as a source of truth and that should never be forced pushed to. With that being said this is the first time anything has accidentally landed since we’ve been working with this setup… it seems reasonable to allow for the one off and simply disable / force push if this happens again.

…

On Nov 20, 2018, at 10:12 PM, Rod Vagg ***@***.***> wrote: Well, in practical terms, releasers own those branches, so you could just clean them up however you like. If someone accidentally lands something on a .x branch then you could just force push it out to clean it up. Since the number of people managing these branches is small I don't think that's going to mean much toe stepping. I wouldn't object if the person doing the next Current took the initiative to clean out anything mistakenly on v11.x, that just seems like a reasonable part of the process, no? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#392 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV7HeNXZuSIBB1VygWa-VCAbg3TUWks5uxMSngaJpZM4Yqs8->.

[targos]

And what about the other argument I made (more engagement in release PRs)?

[BridgeAR]

We definitely need more engagement in release and backport PRs but after thinking about this again, it does seem to be a special situation in which it's just best to allow force-pushing right quick. The only problem about that is that not everyone is allowed to disable force pushing but I guess if that happens, it's fine to ask someone who has the permissions to do so.

Since there was no further discussion about this, I'll close this.

@targos it might be good to bring this topic again up at the next release meeting?