Any other thoughts or comments on this? >From the limited number of "votes", it seems like the preference is for option 2 -- keeping the Assignee field set to the fixer and using other fields to denote the code reviewer and readiness for commit.
If I don't hear any other opinions, I'll work with others to update the wiki page to make this process change. Thanks. -- Zelaine On Tue, Nov 29, 2016 at 11:59 AM, Zelaine Fong <zf...@maprtech.com> wrote: > 1) Yes, we don't have a good system for this. Committers can > watch/monitor and do the assignment, as you've suggested. > > 2) If we go with proposal #2, then yes, adding a comment when the Reviewer > assignment is done would be needed to trigger an email notification. Or we > could make the Reviewer a watcher on the issue. In either case, the > explicit additional step is required. > > Regarding adding new status states to Apache Jira -- I'm not sure how hard > that is. I recall discussion about attempts being made to add new fields > into Apache Jira. But I'm not sure about states. Any one have experience > with this? If it's easy to do, I think that's a better way of tracking the > new state change that proposal #2 needs. > > -- Zelaine > > > On Tue, Nov 29, 2016 at 11:02 AM, Paul Rogers <prog...@maprtech.com> > wrote: > >> Sounds like #2 has a clear lead. Couple of suggestions. >> >> 1. How do we know the reviewer to specify? In many cases, we informally >> know a likely person. Community members may not be sure. Or, can a >> committer watch for such cases and do the assignment? >> >> 2. As it turns out, JIRA does not send an e-mail notification when >> assigning someone as a Reviewer. Instead, perhaps include a brief comment, >> mentioning that person, to force JIRA to alert the Reviewer that they have >> a new item. >> >> As a follow up, anyone know if we can lobby Apache to add status states? >> That is, to add the states that Julian suggested? Typical workflow: Open -> >> In Progress -> Reviewable -> Ready to Commit -> Resolved. >> >> (Actually, if we include QA activities, the flow should be Resolved —> >> Verified. But, that is a different discussion.) >> >> The list of states (visible from clicking Resolve), includes some odd >> items such as “Staged” and “REMIND”, which seem like specialized states >> added for ad-hoc purposes... >> >> Thanks, >> >> - Paul >> >> > On Nov 29, 2016, at 9:23 AM, Julian Hyde <jhyde.apa...@gmail.com> >> wrote: >> > >> > I like 2 also. >> > >> > Is the following variant of 2 possible in JIRA? Make the "ready to >> commit" flag into a status. Thus status changes from "open" to "in >> progress" to "reviewable" to "ready to commit" (or "approved"). >> > >> > The inability to search by assignee should not be a huge problem - it >> is in the interests of the project to ensure that the number of cases in >> "reviewable" or "ready to commit" state at any moment is small. >> > >> > And furthermore, the author of a bug often doesn't know or care who >> will review or commit it. So the very notion of an assignee is suspect. >> > >> > Julian >> > >> >> On Nov 29, 2016, at 07:53, Aman Sinha <amansi...@apache.org> wrote: >> >> >> >> 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 <zf...@maprtech.com> >> 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 >> >>> >> >> >
