Repository navigation
Which teams should be allowed to land pull requests on nodejs/node? #1039
Description
Activity
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.
Reacted by bl-ue, Beth Griggs and Ruben BridgewaterCommComm 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.
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 bygit 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".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.
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.
Reacted by Derek LewisI'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 asmanual-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-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.Reacted by Gireesh Punathil and bl-ueMaybe 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
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.
Reacted by Gireesh Punathil and bl-ueIMO, 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.
Reacted by bl-ue and Beth GriggsReacted by bl-ueperhaps we can do an extra-check where Triagers can only land via commit queue if
author-readylabel is included?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.Reacted by bl-ueIMO 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.
Reacted by Colin Ihrig, Gireesh Punathil and Myles BorinsI'm not going to object to letting triagers land pull requests, but I would like us to try and do two things regardless:
- Add some kind of CODEOWNERS enforcement to the landing process
- Explain the
request-ciandcommit-queuelabels when triagers are onboarded.
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.
Reacted by bl-ue and MesteeryI 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?
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
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.
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.
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.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.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-queue
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.
No objections here, you can let them land nodejs/node#38717 whenever they want. Should be good to test it out for them.
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
⚠️ Could not retrieve the email or name of the PR author's from user's GitHub profile!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)
-
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.
Reacted by Derek LewisReacted by Derek LewisCan this issue be closed now? I did not modify any permissions myself, but assuming that got done, this should be resolved.
Agreed, closing.
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/nodeis 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:
issue-triageteam admin#552