Re: [PROPOSAL] Apache Jira Workflow for Code Reviews

[Zelaine Fong <zf...@maprtech.com>]

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 <pr...@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 <jh...@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 <am...@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
>> >>>
>>
>>
>

Re: [PROPOSAL] Apache Jira Workflow for Code Reviews

[Parth Chandra <pa...@apache.org>]

Option 2 sounds good.

On Mon, Dec 5, 2016 at 8:32 AM, Zelaine Fong <zf...@maprtech.com> wrote:

> 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 <pr...@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 <jh...@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 <am...@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
> >> >>>
> >>
> >>
> >
>