[DISCUSS] Pull Request review and approval policy

[Jason Plurad]

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?

[Alexandr Porunov]

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.

[Jerry He]

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

[Jason Plurad]

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

[Debasish Kanhar]

+1 on Jason's idea.

This helps people who are bit busy with other assignments to have easy way to track the existing PRs needed for approval, and in turn reduces chances of PR reviews getting missed out too.

The more streamlined the things are, the more simpler it makes life of all approvers. :-)

[Alexandr Porunov]

Will there be a new review and approval policy?

I believe it should be implemented.

+1 for all discussed above.

[Jason Plurad]

There hasn't been any negative responses on this thread. I will submit a PR today to update the PR docs to outline these updates in policy.

1. CTR is already part of our policy
  a. Committers are trusted to decide when a PR can be Commit-Then-Review (CTR) vs Review-Then-Commit (RTC)
  b. Include merging PRs upstream as one of the CTR scenarios
  c. GitHub will not be used to enforce the policy (disable approval reviews) because it does not allow committers to approve their own PRs

2. Our RTC policy requires either
  a. 2 approval votes from committers
  b. 1 approval vote from a committer followed by a one week review period
  c. Committers are trusted to seek 2nd approval for complex changes that require more review

[Chris Hupman]

+1. 

I like CTR for upstream merges to help streamline the process. 

I personally really like getting a second set of eyes on my commits and enjoy the feedback. That being said I've gotten some great review recommendations from non-committers in the past (Thanks Porunov) and agree that it makes sense to assume committers will request for additional review when warranted. 

[Henry Saputra]

I am +1 for the suggestions to augment with RTC plus lazy consensus

I am sorry for lag of responses for the proposal.

- Henry

--
You received this message because you are subscribed to the Google Groups "JanusGraph developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to janusgraph-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/janusgraph-dev/faf9fb95-dbd3-42d0-bb24-aa66e54ad06c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jason Plurad]

Thanks Henry. The policy has been updated already. https://github.com/JanusGraph/janusgraph/blob/master/docs/development.adoc

To unsubscribe from this group and stop receiving emails from it, send an email to janusgraph-dev+unsubscribe@googlegroups.com.