[DISCUSS] Pull Request review and approval policy
Jason Plurad <plu...@...>
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:
Thoughts?
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 unchangedBy 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?