Repository navigation
Which teams should be allowed to land pull requests on nodejs/node? #1039
Description
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:
Activity
cjihrig commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
I don't think they have commit access. We have branch protection rules that include administrators.
targos commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
FWIW, only the CommComm chair has Admin on the org, not the entire CommComm team.
mhdawson commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
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.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-trackrequest was 👍 ed by at least two collaborators – IIRC, if a PR is labeled withfast-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 bothfast-trackandcommit-queuelabels on a PR to land "illegal" commits, but it would still require at least 1 collaborator approval and can be easily reverted anyway.nschonni commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
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 commentedon Jun 14, 2021
perhaps we can do an extra-check where Triagers can only land via commit queue if
author-readylabel is included?mhdawson commentedon Jun 16, 2021
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-readysuggestion, 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 commentedon Jun 16, 2021
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 commentedon Jun 23, 2021
I'm not going to object to letting triagers land pull requests, but I would like us to try and do two things regardless:
request-ciandcommit-queuelabels when triagers are onboarded.Trott commentedon Jun 23, 2021
This may start to be getting off topic, but I suspect we also need to explain the
needs-cilabel (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 commentedon Jun 23, 2021
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 commentedon Jun 23, 2021
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 commentedon Jun 23, 2021
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 commentedon Jun 29, 2021
Does anyone object to the creation of a
manual-landlabel? 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 commentedon Jun 30, 2021
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-landis fine, but here are some other ideas:land-manuallydo-not-use-commit-queueno-commit-queuemhdawson commentedon Jul 14, 2021
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 commentedon Jul 17, 2021
No objections here, you can let them land nodejs/node#38717 whenever they want. Should be good to test it out for them.
DerekNonGeneric commentedon Jul 17, 2021
There are still a couple of gotchas to be aware of wrt the commit queue:
a strange error as the one below will appear if there are any unresolved CI failures
this means that the triager needs to re-run the workflow usually (here is how to do so)
getting purple merges is still blocked on build: use purple merge instead of close on CQ node#35001,
so they should anticipate that the landed PRs will be closed and red (normally is this way anyways most of the time)
mhdawson commentedon Jul 22, 2021
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 commentedon Sep 5, 2021
Can this issue be closed now? I did not modify any permissions myself, but assuming that got done, this should be resolved.
mhdawson commentedon Sep 7, 2021
Agreed, closing.