search

nodejs / TSC

The Node.js Technical Steering Committee
595 stars 133 forks source link

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

Closed mmarchini closed 3 years ago

mmarchini commented 3 years ago

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:

cjihrig commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :+1: 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 :+1: 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

mhdawson commented 3 years ago

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.

mmarchini commented 3 years ago

IMO code quality check as well as checking if something should be semver-minor/semver-major should be done during reviews and not while landing. For semver labels I have always trusted other collaborators to have done due diligence in determining semverness if all I'm doing is landing a pull request.

As for "checking for outstanding suggestions": suggestions are not a requirement for landing, if the author is expected to apply suggestions the Request Change feature should be used (we changed the collaborator guide to remove ambiguity around this a while back because leaving to collaborators to interpret if a suggestion is required or not can lead to misinterpretation and tension in the pull request, which has happened before). If folks do think this is an important part of landing a PR though, it seems like an easy thing to include as part of our triagers onboarding process.

targos commented 3 years ago

I'm not going to object to letting triagers land pull requests, but I would like us to try and do two things regardless:

Trott commented 3 years ago

This may start to be getting off topic, but I suspect we also need to explain the needs-ci label (or get rid of it or rename it). Specifically, I'm under the impression it should remain after the CI is run and green because it's not a label that means "this needs a Jenkins run in the future" but rather "this needs a Jenkins run for it to land because it modifies executable code". I think a lot of people think it means that first thing and remove it after Jenkins starts or after it's green.

It may be too late to change this, but I wonder if in general, labels that are primarily for the bot/Actions should be prefixed with something so that it's clearer.

mmarchini commented 3 years ago

I can open a PR to add codeowner requirements to our policy and try to get some automation in place for checking it. I'll also check if the codeowner ping bot is working correctly, I haven't seen any pings lately.

Anyone volunteers to make changes to the triagers onboarding process?

mmarchini commented 3 years ago

It may be too late to change this, but I wonder if in general, labels that are primarily for the bot/Actions should be prefixed with something so that it's clearer.

Should be straightforward to do for CQ/start CI since both are defined in GitHub Actions files. Not sure if we have other labels that are used by automation

richardlau commented 3 years ago

This may start to be getting off topic, but I suspect we also need to explain the needs-ci label (or get rid of it or rename it). Specifically, I'm under the impression it should remain after the CI is run and green because it's not a label that means "this needs a Jenkins run in the future" but rather "this needs a Jenkins run for it to land because it modifies executable code". I think a lot of people think it means that first thing and remove it after Jenkins starts or after it's green.

It may be too late to change this, but I wonder if in general, labels that are primarily for the bot/Actions should be prefixed with something so that it's clearer.

I'm fairly certain that label was repurposed and it was originally meant to be the latter before the contribution process changed to only requiring GitHub action runs for doc-only PRs.

aduh95 commented 3 years ago

Does anyone object to the creation of a manual-land label? I think it would be useful regardless of who can use the CQ. Open to suggestions for a clearer name btw. If no one objects, I plan on opening a PR next week.

Trott commented 3 years ago

Does anyone object to the creation of a manual-land label? I think it would be useful regardless of who can use the CQ. Open to suggestions for a clearer name btw. If no one objects, I plan on opening a PR next week.

Sounds good to me. I don't think you need to wait on determining the name since it will be easy enough to rename the label and update the docs. Here are my thoughts on a name:

manual-land is fine, but here are some other ideas:

mhdawson commented 3 years ago

In the TSC meeting today we discussed the issue, we had 9 TSC members in attendance and nobody objected to letting triagers land using the commit queue. We felt that we should try it out and can adjust if there are any issues. @nodejs/tsc if you have any concerns please comment otherwise if we don't have any objections from people who make the next meeting the assumption is that triagers land with commit queue will be ok.

DerekNonGeneric commented 3 years ago

No objections here, you can let them land https://github.com/nodejs/node/pull/38717 whenever they want. Should be good to test it out for them.

DerekNonGeneric commented 3 years ago

There are still a couple of gotchas to be aware of wrt the commit queue:

mhdawson commented 3 years ago

There were 11 TSC members in the meeting today. No objections and non from TSC members in issue, so ok from TSC perspective for triagers to land using commit queue, removing from commit queue.

DerekNonGeneric commented 3 years ago

Can this issue be closed now? I did not modify any permissions myself, but assuming that got done, this should be resolved.

mhdawson commented 3 years ago

Agreed, closing.