Landing PRs policy

[mmarchini]

I just realized we don't have a policy for landing Pull Requests and I guess most of us are assuming Node.js core policies here: at least one approval + wait 48/72 hours. Considering the project has only a few collaborators, I don't think this policy is adequate since it usually takes weeks to get approvals.

Inspired on recent discussions about PR waiting time on Node.js core (nodejs/node#22275) and on the Commit-Then-Review concept, I'd like to propose the following PR policy:

Before landing a pull request, make sure the following conditions must be true:
- The Pull Request must contain tests, when applicable
- Last CI run must be green
- There must be no objections (GitHub's Request for Changes)

Also, at least one of the following conditions must be true:
- The Pull Request is open for at least 72 hours
- The Pull Request was approved by at least one collaborator

I'm open to discuss this in person next week during the Collab Summit :)

cc @nodejs/diagnostics since llnode falls under the WG Governance. Also ccing llnode collaborators: @joyeecheung @indutny @cjihrig @bnoordhuis @rnchamberlain @hhellyer

[richardlau]

The one advantage of following the same rules as core is that the associated tooling (i.e. node-core-utils) should just work. That's not to say we couldn't teach ncu different rules for different projects (but then we'd have to make sure it applied the correct rules).

[mmarchini]

node-core-utils already have problems with llnode: it complains because there's no CI run (we're not using Jenkins yet) and users have to use ncu-config to make ncu look into Node.js README for the list of collaborators (which doesn't make sense, llnode collaborators are not necessarily Node.js collaborators). Thus I don't think this should be a reason to keep following core's policy.

By the way, I made a mistake in my first comment: right now core requires two approvals instead of one, unless the pull request is open for over a week. This rule works for core where we have several collaborators who review pull requests daily, but it is excessive for a project with only a handful of active collaborators.

---

With that being said, teaching ncu different rules according to the project sounds like a good idea. Maybe we could have a .ncurc in our projects, where we can customize ncu's behavior accordingly?

For example:

{
 "wait-time": 72,
 "min-approvals": 2,
 "required-ci": [ 
{"type": "jenkins", "job": "node-test-pull-request" }, 
{"type": "jenkins", "job": "node-test-pull-request-lite-pipeline" }
 ]
}

[joyeecheung]

@mmarchini You maybe looking for the ./.ncu/ directory as that's where ncu-config serialize per-project configurations already? (e.g. ncu-config set repo llnode is serialized in ./ncu/config). Also, PRs to refactor the PR checker in ncu (ideally configurable through rules like what you described) are very welcomed!

which doesn't make sense, llnode collaborators are not necessarily Node.js collaborators

The llnode collaborators are still a subset of node collaborators at the moment, but yeah we need something for the long term. I thought about moving the collaborator information from the README (which is weird to parse) to a JSON, but the idea of moving names out of the README wasn't quite popular in core - although simply duplicating the information in a JSON is probably not going to meet much objections, I guess.

[mmarchini]

maybe looking for the ./.ncu/ directory

./.ncu/ is being used for user-specific settings, right? I suggested .ncurc so we could have something committable, then we could have per-project settings applicable for all users.

[joyeecheung]

@mmarchini We could have .ncu/something committed into the tree, and ask users to put something like

.ncu
!.ncu/something

into their global git configs. But yeah it also make sense to make this a top-level file. Maybe .ncuattributes? (Thinking along the lines of .gitattributes)

[mmarchini]

@joyeecheung ncu changes aside, what do you think about the proposed policy?

[joyeecheung]

@mmarchini IIUC, the proposed policy allows PRs to be merged without reviews after it is open for 72 hours? I don't feel quite comfortable with this, considering there are actual users of this project who are not our collaborators, to be honest..
(I would be a bit more comfortable if it's 7 days and the PR is opened by an existing collaborator, but it's just me, though)

[mmarchini]

I would be a bit more comfortable if it's 7 days and the PR is opened by an existing collaborator, but it's just me, though

That sounds reasonable to me.

[mhdawson]

I guess only collaborators can land PRs. Maybe something like allowing a collaborator to approve their own PR after 7 days ...

[mmarchini]

I like this idea @mhdawson. I'll open a PR to document this and cc @nodejs/diagnostics there.