[PROPOSAL] Apache Jira Workflow for Code Reviews

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

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

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

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

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

[Paul Rogers <pr...@maprtech.com>]

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

[Julian Hyde <jh...@gmail.com>]

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

[Aman Sinha <am...@apache.org>]

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
>