Diff for "WorkingWithReviews"

Not logged in - Log In / Register
Differences between revisions 1 and 2
⇤ ← Revision 1 as of 2009-05-25 09:47:51 →
Size: 8239
Editor: sinzui
Comment:
← Revision 2 as of 2009-05-25 10:23:31 → ⇥
Size: 5973
Editor: sinzui
Comment:
Deletions are marked like this. Additions are marked like this.
Line 3: Line 3:
Line 8: Line 9:
Line 11: Line 13:
Line 16: Line 19:
Line 19: Line 23:
See https://help.launchpad.net/Code/Review to learn about Launchpad Merge Proposals.
Line 20: Line 25:
'''Please be sure to use the [[LPReviewPluginDocs|bzr review-submit]] plugin to submit your branch for review.'''
Line 24: Line 28:
The story starts with you putting your branch in the general queue on PendingReviews. From there the branch is assigned to a reviewer within 1 working day. At this point you can nag your reviewer effectively until he reviews your branch. The story starts with you making a Launchpad Merge Proposal into launchpad-devel or launchpad-db-devel. From there the branch is queued for review at https://code.launchpad.net/launchpad/+activereviews . You may ask an on-call reviewer in #launchpad-reviews to review the branch outside of the queue.
Line 26: Line 30:
Once your reviewer has had time to go over the modifications, he will email you back, CC: launchpad-reviews, with a description of areas or items that need to be fixed, modified or improved in your code. He may also write special recommendations, commend you for your code quality, or ask questions if unclear on domain specifics or obscure functionality. Once your reviewer has had time to go over the modifications, he will reply to the proposal with a description of areas or items that need to be fixed, modified or improved in your code. He may also write special recommendations, commend you for your code quality, or ask questions if unclear on domain specifics or obscure functionality.
Line 35: Line 39:
Line 48: Line 53:
    * needs-review     * needs review
Line 50: Line 55:
        The initial status. Also indicates a branch which is pending
        further review feedback from a reviewer.		         The initial status.
Line 53: Line 57:
    * needs-reply     * needs fixing
Line 55: Line 59:
        The branch has been reviewed and awaits a
        response from the code/patch author.		         The branch has been reviewed and requires some small changes.
Line 58: Line 61:
    * merge-conditional     * resubmit
Line 60: Line 63:
        The branch has been approved for merging pending comments or
        minor fixes that the review pointed out in his review message.		         The branch has been reviewed and it has fundamental problems that require substantial changes '''after''' a new implementation call.
Line 63: Line 65:
    * merge-approved     * rejected
Line 65: Line 67:
        The branch has been OK to be merged, unconditionally.         The focus/intent of the branch is not appropiate for merging.
Line 67: Line 69:
    * work-in-progress     * approved (with a comment)
Line 69: Line 71:
        The branch is not ready for review, or has been pushed back out of
        review into this state. It is listed for convenience, so people can
        find relevant branches, and see the current diff.		         The branch can be merged '''after''' the reviewer's comments are addressed.
Line 73: Line 73:
Also, if your branch was reviewed by a mentored reviewer (a.k.a. a recruit),
your review status will have a '*' appended to it. This means that the mentor
has not yet signed off on the review. '''You may never land a starred
branch.''' Once the recruit's mentor has signed off on the branch, the
recruit will remove the star and then the review status may be acted on. It's
your responsibility to ping your reviewer if your branch is sitting in
`merge-approved*` or `merge-conditional*`. That way, your reviewer can bug
his mentor to perform the oversight sign off.		     * approved
Line 82: Line 75:
There is usually a cycle between `needs-review` and `needs-reply` and
finally ending with one of the merge tags. When you have replied to the
reviewer, change the tag to `needs-review`; when the reviewer has reviewed
(and possibly re-reviewed) the branch, he will move it back to
needs-reply. Spiv's diagram summarizes:		         The branch can be merged.

Also, if your branch was reviewed by a mentored reviewer (a.k.a. a mentat), you require another review.
before you can merge.
Line 89: Line 82:
                             .--------------------.
                             | |
                             v |
                     .--------------. .-------------.
        START ---> | needs-review | ---> | needs-reply |
                     '--------------' '-------------'
                            /|\
 .-------------------. / | \ .----------------.
 | merge-conditional | <--' | '--> | merge-approved |
 '-------------------' | '----------------'
                             |
                    .------------------.
                    | work-in-progress |
                    '------------------'		                                  .--------------------.
                                 | |
                                 v |
                         .--------------. .--------------.
            START ---> | needs-review | ---> | needs-fixing |
                         '--------------' '--------------'
                                /|\
 .-----------------------. / | \ .----------.
 | approved-with-comment | <--' | '--> | approved |
 '-----------------------' | '----------'
                                 |
                           .----------.
                           | rejected |
                           '----------'
Line 105: Line 98:
Line 114: Line 108:
Line 116: Line 111:
Each merge message '''must''' include an item indicating who the reviewer of the change was. There are a few special-cases involved, so the rules for the merge message are detailed below individually: Each merge message '''must''' include an item indicating who the reviewer of the change was. This is the general form of a commit message.
Line 118: Line 113:
  * If the modification went through the normal review process, you should add an
  {{{
    r=fooReviewer }}}
  string to your merge summary.		   * {{{[r|rs=foo,bar][ui=none|baz|rs][bug=nnnnnn] describe the change.}}} <<BR>> 'r' means review, 'rs' means rubberstamp. One or ore reviewers must follow the r or rs. The 'ui' token states who reviewed the ui; none may be used when there is no ui, and rs may be used when there is ui, but it needs review '''after''' merging.
Line 123: Line 115:
  * If multiple reviewers were involved, you may use the form
  {{{
    r=fooReviewerA,fooReviewerB }}}
    in your message. Optionally, you can be more precise and say something similar to
  {{{
  r=fooReviewerA for modifications to frobbing, and r=reviewerB for the baznification changes }}}		   * {{{[testfix]}}} is a special leading token required to land a branch when trunk branch is broken and future merges must supply a fix.
Line 130: Line 117:
  * If your reviewer has said that the changes may be landed without specific or in-depth code review -- IOW, he did not look at the diffs and comment and approve them directly, use an rs (Rubber Stamp) string:
  {{{
    rs=fooReviewer }}}
  Rubber stamps should be rare and only applicable in special cases, or when the change is trivial.

  * If you have a one or two-line patch that doesn't seem to require review, or for which a review is deemed too onerous, you should attempt to seek a fast review over IRC. Failing that, you may use a [trivial] marker:
  {{{
    [trivial] Frob the baznified parachunkulators. }}}
  as the first item in your merge summary. Trivial merges are intended to be rare and should be reviewed if possible, even if over IRC.
Line 147: Line 125:

where {{{[r=reviewer]}}} is the most common case, but may be substituted with any review status from above as appropriate. Here's an example:

{{{
bzr pqm-submit -m'[r=BjornT] Fix bug 523.'
}}}

If you like being extra cautious, you might want to use the {{{--dry-run}}} option first, just to check that everything is in order before submitting:

{{{
bzr pqm-submit --dry-run -m'[r=BjornT] Fix bug 523.'
}}}

=== After you've merged ===

Once your branch or patch has landed, you should visit PendingReviews and remove the branch.

Describe WorkingWithReviews here.This page covers the process for effectively dealing with code and design reviews.

Contents

  1. Overview
    1. Specification reviews
    2. Design reviews
    3. Code reviews

Overview

During the development of a feature from concept to rollout there are a number of reviews that occur. The first review process is the SpecificationProcess where the concept is examined and refined into a plan. This plan is eventually signed off by the appropriate manager. Some time later, a coder will start to implement it. When they are about to do this a DesignPhoneCall takes place, which is a verbal review of the design of the planned implementation. Finally, once the code is complete the PreMergeReviews process takes place.

Specification reviews

Please see SpecificationProcess for the specification workflow.

Design reviews

Design reviews are conducted by a DesignPhoneCall between a coder and a reviewer. The review aims to ensure that the resulting code will have few or no structural problems, which makes review of the code easier and faster.

Code reviews

Once your code is complete, it must go through the PreMergeReviews process to be accepted into the central trunk. See https://help.launchpad.net/Code/Review to learn about Launchpad Merge Proposals.

Handling review responses

The story starts with you making a Launchpad Merge Proposal into launchpad-devel or launchpad-db-devel. From there the branch is queued for review at https://code.launchpad.net/launchpad/+activereviews . You may ask an on-call reviewer in #launchpad-reviews to review the branch outside of the queue.

Once your reviewer has had time to go over the modifications, he will reply to the proposal with a description of areas or items that need to be fixed, modified or improved in your code. He may also write special recommendations, commend you for your code quality, or ask questions if unclear on domain specifics or obscure functionality.

You must respond to each individual suggestion and question made by your reviewer; one of the main dangers with reviews is wasting the reviewer's time by ignoring or forgetting to deal with some specific issue, and the reviewer can't be expected to go through and ensure each item was dealt with --- please make an effort to ensure you respond accurately to reviews.

One or more review cycles will be necessary per-modification; the number of cycles will depend on the amount of code changed, the increasing familiarity of the reviewer with your code, and of course the quality of the code produced.

You may only merge once the reviewer has approved the modification.

Small branches are faster to review than large ones - a branch that is twice as big as another takes more than twice as long to review. Please try to keep branches small and focused.

Review tags

During the review process, you should keep your branch tags updated correctly in

The tags are meant to give better feedback of where the review stands, and will allow the review status page to show the branches clearly.

The tags may assume one of the following strings:

  • needs review
    • The initial status.
  • needs fixing
    • The branch has been reviewed and requires some small changes.
  • resubmit

    • The branch has been reviewed and it has fundamental problems that require substantial changes after a new implementation call.

  • rejected
    • The focus/intent of the branch is not appropiate for merging.
  • approved (with a comment)

    • The branch can be merged after the reviewer's comments are addressed.

  • approved
    • The branch can be merged.

Also, if your branch was reviewed by a mentored reviewer (a.k.a. a mentat), you require another review. before you can merge. 

                                 .--------------------.
                                 |                    |
                                 v                    |
                         .--------------.      .--------------.
            START --->   | needs-review | ---> | needs-fixing |
                         '--------------'      '--------------'
                                /|\
 .-----------------------.     / | \     .----------.
 | approved-with-comment | <--'  |  '--> | approved |
 '-----------------------'       |       '----------'
                                 |
                           .----------.
                           | rejected |
                           '----------'

Dealing with Conflicts

If, after a successful review process, you submit your code to PQM only to find conflict you need to resolve the conflicts:

  1. Merge rocketfuel onto your reviewed branch, resolve the conflict, push, and submit another merge request.

If, in the process of resolving conflicts, you make non-trivial changes to the code, you should consider having those changes reviewed before attempting to merge them.

The merge message

Each merge message must include an item indicating who the reviewer of the change was. This is the general form of a commit message.

  • [r|rs=foo,bar][ui=none|baz|rs][bug=nnnnnn] describe the change.
    'r' means review, 'rs' means rubberstamp. One or ore reviewers must follow the r or rs. The 'ui' token states who reviewed the ui; none may be used when there is no ui, and rs may be used when there is ui, but it needs review after merging.

  • [testfix] is a special leading token required to land a branch when trunk branch is broken and future merges must supply a fix.

Make it so

When you are ready to submit your branch to pqm for merging, use the following command (from RocketFuelSetup): 

bzr pqm-submit -m '[r=reviewer] description of changes'

WorkingWithReviews (last edited 2011-02-17 20:01:28 by lifeless)