Re: [DISCUSS] Pull Request review and approval policy


Jason Plurad <plu...@...>
 

Perhaps a weekly update on the PR queue posted on the dev mailing list would be a good idea.

Something like:

# 0.2.2 queue
PR a - needs one more approval
PR b - waiting on revisions
PR c - unreviewed for 16 days
PR d - new this week


On Thursday, August 9, 2018 at 1:17:40 AM UTC-4, Jerry He wrote:
Finding the right balance between more review/more approvals and being more efficient is what we aim for.  I have been in situations in my PRs where another pair of eyes caught some error I made.  I was also in situations where my PRs sitting there for a long time waiting for reviews.

I think I am ok with adopting 2-4 from the TinkerPop process. 
But adding some extra guard (e.g an extra reminder or ping before commit) or time (more time than a week to accommodate busy professionals?)  will make it easier to relieve concerns.

Thanks,

Jerry


On Wednesday, August 8, 2018 at 1:14:06 PM UTC-7, Alexandr Porunov wrote:
I am not a committer but here are my thoughts:

1. I think it is enough to maintain 2 active branches right now. Latest release + future release (master). There is plainly no enough committers to maintain more active branches.

2. I think it is worth adding steps 2-4 because there are some PRs with huge delays.

On Wednesday, August 8, 2018 at 6:25:49 AM UTC+3, Jason Plurad wrote:
I have a couple discussion topics to bring up based on our recent history of getting releases out.

1. Based on what we have written in the docs (Chapter 44. Pull Requests), we have a commit-then-review (CTR) policy, however it does not work in practice. GitHub does not allow a committer to approve their own PRs. This makes it impossible to merge PRs without at least 1 other approval, and this has caused delays on some PRs to get merged. For example, the master branch is not yet open for business because the PR is still waiting for approval. Another situation was when we needed to merge a fix from 0.2 upstream into master. The 0.2 PR got approved and merged, but either the committer didn't open a separate PR to merge to master or the committer opened the PR to merge to master but that PR stagnated on getting approval. If we end up in a situation where we are maintaining more than 2 active branches, there would be even more friction involved getting changes merged upstream.

Based on these docs, we can disable restriction checks for repository administrators. If we still want to allow CTR, it seems like this needs to be done and all committers would need to be in a repository administrator group.


2. Currently our policy is for 2 committer approvals to merge a PR. Toward the end of the release cycle, I had gotten in the habit of performing my review approval first and then requesting reviews from the JanusGraph/committers group via GitHub. This should send a notification to all the committers in the group. I have no idea how other committers manage their GitHub notifications, but it seemed like a better approach than spamming the dev list or backchanneling individuals every time a PR needed a second review.

TinkerPop recently introduced a new approach to their PR review process:
We seem to agree that in the future we will go with the following review-the-commit (RTC) process:

1. Each change to TinkerPop release branches requires 3 +1s from committers and no -1s OR
2. A single +1 from a committer and a 1 week review period for objections (i.e. "cool down period") at which point we will assume a lazy consensus
3. The single +1 may come from the committer who submitted the PR in the first place
4. Committers are trusted with their changes but are expected to request reviews for complex changes as necessary and not rely strictly on lazy consensus
5. commit-the-review (CTR) procedures remain unchanged
By adopting bullets 2-4 from TinkerPop's process, I think it would enable us to merge changes into the codebase more efficiently. If committers were concerned about what PRs might get merged, they would need to check the PR queue at least once a week.


Thoughts?

Join janusgraph-dev@lists.lfaidata.foundation to automatically receive all group messages.