My preference is for #2 because of the reasons you mention. For #1 my recollection is the DrillCommitter was used at a time when there were only couple of committers. Today there are many. Changing to DrillCommitter will leave ambiguity about which committer should look at it. Additionally, I feel that today we are not using the field 'Reviewer' for its correct purpose. This argues for #2.
One note about #2 worth mentioning is that folks would have to change their search criteria to 'assigned *OR reviewer*' to find out what is on their plate. On Mon, Nov 28, 2016 at 4:51 PM, Zelaine Fong <[email protected]> wrote: > I'd like to propose a small change to the current process for code reviews > in Apache Drill. We need this change because non-committers are becoming > more involved in code reviews. > > The current process is described at > https://drill.apache.org/docs/apache-drill-contribution- > guidelines/#step-3:-get-your-code-reviewed-and-committed-to-the-project. > It assumes that the individuals doing code reviews are code committers. > Here's a brief summary of the process: > > *Current Process:* > Currently, the workflow is based on updating the Status and Assignee > fields. When a pull request has been posted and is ready for review, the > issue status is changed to "Reviewable" and the Assignee is changed to the > designated code reviewer. If the review is completed and the reviewer is a > committer, the reviewer/committer will commit the change. If the code > reviewer has comments that require the contributor to address, the issue > status changes back to "In Progress" and the Assignee is changed back to > the contributor. > > *Proposed Change:* > The proposed change is to address the case where another step may be needed > to commit changes, if the changes have been reviewed by a non-committer. > Here are a couple of proposals on how to address this. In both cases, once > a review has been satisfactorily completed on a pull request, a committer > will take over, bless the changes based on the prior review comments, do a > +1 on the patch, and then commit. > > > *Proposal #1:*When a fix is ready to be reviewed, set the Assignee to > "DrillCommitter". > > Pros: Simple. User "DrillCommitter" already exists. > Cons: Similar to changing the Assignee to the code reviewer, you lose track > of who is the original contributor of an issue. Yes, it's in the comment > history, but there is no automated/easy way to get this info. > > *Proposal #2:* > Leave the Assignee field unchanged. This is also means changing the > current process of changing the Assignee to the code reviewer. Instead, > > 1) Continue to set the Status to "Reviewable" to indicate that the fix is > ready for review, but set the Reviewer field to the designated code > reviewer. > 2) If changes are needed after code review, change the status of the issue > back to "In Progress" as today. But again, don't change the Assignee. > 3) If the changes are acceptable, the reviewer applies the > "ready-to-commit" tag on the Jira. > > Pros: Maintains Assignee field > Cons: Requires querying for the tag "ready-to-commit" to find commit-ready > fixes. > > Please comment with your preferences for proposal 1 or 2, or if you have > other suggestions. > > Thanks. > > -- Zelaine >
