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 > >>> > >
