Which teams should be allowed to land pull requests on nodejs/node?

[mmarchini]

The introduction of a commit queue made it possible for teams without commit access to land pull requests on Node.js. While commit privilege was not limited to collaborators before (CommComm and Moderation teams have commit access due to Admin privileges), there was an implicit understanding that only the Collaborators team was allowed to land pull requests (implicit because it's not written anywhere in the collaborator guide that only collaborators are allowed to land pull requests).

Some discussion around the topic happened before the commit queue was landed, and as the person leading those discussions, my understanding back then was that it was decided that with the commit queue other teams would be allowed to land pull requests too (because the commit queue ensures everything we need to land a PR is met before landing it). This was never documented though, and since this discussion resurfaced again I noticed that I might've not been clear during those discussions and might've misunderstood our decisions back then.

I propose we reach a decision on who should be allowed to land pull requests and in which situations. My proposal is:

"Anyone with enough permissions to label pull requests on nodejs/node is allowed to use the Commit Queue to land Pull Requests. If the Commit Queue fails, the Pull Request needs to be landed manually. Only Collaborators are allowed to land Pull Requests manually."

Because the commit queue already runs every check we require on Landing Pull Requests in a very conservative way, which includes approval by Collaborators, it is safe for anyone we already trusted with labeling privilege to land pull requests.

cc @nodejs/tsc

References from previous discussions:

• create a issue-triage team admin#552
• Node.js Technical Steering Committee (TSC) Meeting 2020-08-13 #907 (comment)
• build: implement a Commit Queue in Actions node#34112 (comment)
• 2020-08-13 TSC Meeting Notes

[cjihrig]

I originally thought that we should restrict using the commit queue to collaborators with a commit bit. After thinking about it some more, I think @mmarchini's proposal would work fine in the overwhelming majority of cases, so +1.

[targos]

CommComm and Moderation teams have commit access due to Admin privileges

I don't think they have commit access. We have branch protection rules that include administrators.

[targos]

I personally think that landing a pull request (manually or via the commit-queue) is not a light action.
There are a number of rules that are verified by git node land, but before landing a PR myself, I make sure there are no outsdanding comments and I do a small review on it (look at the code, check that it's not missing the semver-major or semver-minor label...).
The bar to become a triager is very low and I do not feel comfortable letting non-collaborators do this last "quality control".

[jasnell]

Like @targos, I generally do not solely rely on the automation here. There are plenty of times where everything looks like it's ready to land but still should be held up. I prefer that only fully onboarded contributors are able to trigger landing a PR.

[Trott]

CommComm and Moderation teams have commit access due to Admin privileges

FWIW, only the CommComm chair has Admin on the org, not the entire CommComm team.

[mhdawson]

My initial thought is similar to @targos, I think we want personal judgement/review before landing. Given that those with the commit bit are those we have decided to trust with that final assessment I'm thinking at least by policy that should be the same when using the commit queue as well.

[aduh95]

I'm +1 to keep the current system and document somewhere that triagers are allowed to use CQ. I think the quality check can be done when approving the PR rather than when landing.
We may also consider adding another label (such as manual-land) to opt-out of the CQ – and restrict that label removal to collaborators only maybe.

Because the commit queue already runs every check we require on Landing Pull Requests in a very conservative way, which includes approval by Collaborators, it is safe for anyone we already trusted with labeling privilege to land pull requests.

The only time when the CQ is not being conservative is with fast-tracked PR: I think there is no check to validate if a fast-track request was 👍 ed by at least two collaborators – IIRC, if a PR is labeled with fast-track, ncu (and the CQ) will simply skip the 48h/7days check without checking who 👍 ed the comment. So I suppose a triager could add both fast-track and commit-queue labels on a PR to land "illegal" commits, but it would still require at least 1 collaborator approval and can be easily reverted anyway.

[nschonni]

Maybe too much overhead, but the existing label workflow could be amended to confirm that they're a contributor like in https://github.com/github/docs/blob/main/.github/workflows/confirm-internal-staff-work-in-docs.yml#L28-L41, or remove the label if they don't

[joyeecheung]

I think the idea of allowing triagers to land the PRs as long as it passed CQ checks is good. Chromium (including V8) also does something similar - whoever with try job access can land the PR as long as they have approval from owners, even if they are not committers. Although unlike Chromium, we don't enforce the owner system that much, so potentially any collaborator can allow any part of the code base to be changed, even if they have no experience in the code that they approve. But in reality I think collaborators usually exercise their power with discretion and people usually only approve the code that they think they can review, so I am fine with this.

[gireeshpunathil]

IMO, PRs carry shared responsibility among many members of the project, at differing levels of capacity - author(s), triagers, reviewers, landing personal... While landing is a complex process that needs due diligence, treating it as the critical quality control gate may be like diminishing the importance of other roles? The very reason CQ exists is that there are automation possible at that stage. Unless there are incidents of role abuse, I am not keen on removing the capability. On the other hand, this provides more empowerment to triagers and reduces entry barrier to the project.

[MylesBorins]

perhaps we can do an extra-check where Triagers can only land via commit queue if author-ready label is included?

[mhdawson]

My thought go back to this line in the collaborators guide:

Approving a pull request indicates that the Collaborator accepts responsibility for the change.

I think most often collaborators who land a change approve it first and therefore should feel responsibility for the change along with the rest of the collaborators who approved. I think that sense of responsibility is a good thing, and though its good that the commit queue makes it less work to land a PR, I think we want to maintain that responsibility in the person who has landed the PR. We work in a distributed manner and I often see LGTM with suggestions, after X is fixed etc.

@MylesBorins in the case of the author-ready suggestion, if a collaborator is going to add that why not just kickoff the commit queue instead as based on the suggestion that could result in the same thing.