doc: avoid merging other collaborator's PRs

[Fishrock123]

Checklist

• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

doc,meta

Description of change

refs: #6196 (comment)

This has been implicit for a considerable amount of time.

r=@nodejs/ctc

[Fishrock123]

npm patches almost always have a very clear either "good" or "oops broken" state

[yorkie]

@Fishrock123 are CTCs able to merge the pull request of non-CTC collaborator?

[jasnell]

I generally see no point in this. If any PR is ready to merge, then any collaborator should feel free to merge it. If it's not ready to merge, then label it as not being ready (using the in progress label) or update the PR / add a comment indicating that it's not ready. The part about giving the author a ping to see if it's ready is fine, but the rest just feels like pointless process.

[Fishrock123]

then label it as not being ready

Fwiw default is usually not being ready, and that pre-dates the current in progress label..

[addaleax]

@Fishrock123 did you close this because you feel that it is resolved, by agreeing on the label or otherwise? or just because it failed to gain traction?

[Fishrock123]

Apparently, we've implicitly decided that this is not the case. I still don't think it is a great choice.

[addaleax]

@Fishrock123 well … I think @imyller asked basically the specific question that this is trying to answer on IRC a few days ago, so whatever is “decided” should still be documented, because it definitely isn’t obvious for relatively new collaborators.

I’m okay with reopening this, and I think I agree with its content (although I do feel that the in-progress label sends out a clearer message and that should maybe be documented, too).

[jasnell]

In general, if a PR is sitting for a number of days, has sufficient LGTM's, green CI and is obvious that it's done, it should be good to go as far as anyone landing. Otherwise, if a particular contributor just doesn't want their PRs landed by anyone else, then something like the in progress label or an explicit don't land this yet type statement would be best. Or even just expressing a preference that no one else land their stuff -- @Fishrock123, for instance, expressed to me that he preferred if I didn't land his PRs so when I go through looking for things that are ready to land I automatically exclude any PR written by him.

[Fishrock123]

I'm not actually sure that is true though. There was a sizeable history previously where that wasn't the agreed-upon case. The onboarding documentation (previously?) stated this.

[Fishrock123]

e.g. when I was onboarded, that was what was communicated, and I also onboarded other people with that understanding.

[Trott]

I'd like to suggest an alternative to instructing people to ask other Collaborators, as that can run into at least two problems:

• may result in conversation in IRC or email when it really should be on the issue tracker
• suggests that one needs to wait for a response, which is a surefire way for things to get stalled indefinitely

Instead, I'd recommend something along these lines:

Leave a comment in the pull request saying something like, "This looks ready to go! I'm going to land it after 24 hours unless a Collaborator indicates that they think it's a bad idea! Thanks!"

I'd be fine with something like that, but I'd much prefer a policy that leaves things to the judgment of the person wanting to land stuff, though. Like, trivial PRs don't require that sort of process. Large changes that alter a ton of C++ and JS code and require test changes...they require a comment like that. Something like this:

• It's totally OK to land stuff that has the LGTMs, CI, has been open for 48/72 hours, and has no in progress or other do not land indicators. This is especially true for simple changes.
• If you have concerns about landing other people's stuff, it's a totally awesome thing to do to leave a comment saying something like, "This looks ready to go! I'm going to land it after 24 hours unless a Collaborator indicates that they think it's a bad idea! Thanks!" Definitely check in like that if the change is a complicated one.

[Trott]

Two minor notes:

• The 24 hours is me just picking a number. We'd probably want one number for "during the week" and another larger number for "weekends". (Which also brings up the issue that we may wish to clarify at some point what "on the weekend" means as we are a global project and one timezone may be Friday while another is Saturday. But let's have that conversation somewhere else.)
• Maybe throw in an additional, "When in doubt, leave that comment!" thing. Like, don't spend a ton of time figuring out if you should leave the comment or not. If you're not sure, leave the comment and move on to the next thing until the 24 hours (or whatever) have passed.

[imyller]

One solution to consider is the use of "Assignees" for a PR:

I avoid taking a PR for landing prep that some other collaborator has already self-assigned; and likewise I've taken the habit of self-assigning any PR for myself when actively preparing it for landing.

Could we agree on respecting such assigment by collaborators? Including the one from the PR author.

If yes, then for those who don't want own PRs landed by others: just self-assign.

[imyller]

Also, I'd like to add that using collaborator assignment as "who intends to land" control:

• Would not require any changes to existing usage or purpose of labels
• Would also make unnecessary majority of question-and-responses about who's landing and who's not.
• Would not require introducing additional 24/48/72 hour rules for "who's landing?" discussions. Current landing timeframes would continue to apply as-is.
• Would clearly indicate also to outside contributor who is the Foundation member responsible for taking their PR to landing and also available for possible questions regarding that process.

[fhinkel]

I think if you see a (non-controversial) PR with green CI and LGTMs it should be OK for anybody to merge it. Anything else just creates a lot of extra work (several people looking at the same issue over and over). I personally very much appreciate it, if my changes are merged.

We have more than 250 open PRs. I think we should make it as easy as possible to keep this number small.

[mhdawson]

I'll echo @fhinkel's comment. I'm happy if somebody else lands my PRs once they are ready to go an I've not had time to do it yet.

[jasnell]

Closing given the lack of further activity on this.