drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Parth Chandra <par...@apache.org>
Subject Re: [PROPOSAL] Apache Jira Workflow for Code Reviews
Date Mon, 05 Dec 2016 20:56:47 GMT
Option 2 sounds good.

On Mon, Dec 5, 2016 at 8:32 AM, Zelaine Fong <zfong@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 <zfong@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 <progers@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.apache@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 <amansinha@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 <zfong@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
> >> >>>
> >>
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message