launchpad development|
Size: 8239
Comment:
|
← Revision 14 as of 2011-02-17 20:01:28 ⇥
Size: 7014
Comment: a lot has changed
|
| Deletions are marked like this. | Additions are marked like this. |
| Line 1: | Line 1: |
| Describe WorkingWithReviews here.This page covers the process for effectively dealing with code and design reviews. | This page covers the process for effectively dealing with code and design reviews. |
| Line 3: | Line 3: |
| Line 6: | Line 7: |
| 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. | During the development of a feature from concept to rollout there are a number of reviews that occur. The first review process is the LaunchpadEnhancementProposalProcess 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. |
| Line 8: | Line 16: |
| == Specification reviews == | |
| Line 10: | Line 17: |
| Please see SpecificationProcess for the specification workflow. | == Feature review == Consult the LaunchpadEnhancementProposalProcess and the [[UI/Design|UI design]] pages to get the workflow that should be completed before starting work on changes to the UI. |
| Line 14: | Line 25: |
| 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. | 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. |
| Line 18: | Line 33: |
| Once your code is complete, it must go through the PreMergeReviews process to be accepted into the central trunk. | 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. |
| Line 20: | Line 38: |
| '''Please be sure to use the [[LPReviewPluginDocs|bzr review-submit]] plugin to submit your branch for review.''' | |
| Line 24: | Line 41: |
| 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. If you are a reviewer and choose not to have it reviewed by another reviewer, you can simply approve your merge proposal and land it at this point. |
| Line 26: | Line 47: |
| 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, they will reply to the proposal with a description of areas or items that can be fixed, modified or improved in your code. They may also suggest better spellings of things in your code, or get into a discussion about domain specifics or obscure corner cases which they don't have enough personal knowledge of. |
| Line 28: | Line 51: |
| 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. | You '''must''' respond to '''each''' individual suggestion and question made by your reviewer; one of the main dangers with reviews is wasting everyones time by causing lots of back and forth discussions --- please make an effort to ensure you respond accurately to reviews. |
| Line 30: | Line 56: |
| 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. | 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. |
| Line 32: | Line 61: |
| You may only merge once the reviewer has approved the modification. | You may only merge once the reviewer has approved the review. If you want to make a case why a specific suggestion should not be followed then you must bring it up with the reviewer and not merge until agreement has been reached and documented in the merge proposal. |
| Line 34: | Line 66: |
| 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. | 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. |
| Line 36: | Line 70: |
| === Review tags === | |
| Line 38: | Line 71: |
| During the review process, you should keep your branch tags updated correctly in | === Mentored reviews === |
| Line 40: | Line 73: |
| PendingReviews | When developers apply to become Launchpad reviewers they are assigned a mentor. We began referring to the person being mentored as a ''mentat'', a made-up word you may see in reference to mentoring. |
| Line 42: | Line 77: |
| The tags are meant to give better feedback of where the review stands, and will allow the [[https://chinstrap.ubuntu.com/~jamesh/pending-reviews/|review status page]] to show the branches clearly. |
If your merge proposal is reviewed by a ''mentat'' his/her ''mentor'' will need to review the ''review''. The mentat should add his mentor to the merge proposal. You must wait for both to approve the merge proposal before merging it. |
| Line 46: | Line 82: |
| The tags may assume one of the following strings: | === UI reviews === |
| Line 48: | Line 84: |
| * needs-review The initial status. Also indicates a branch which is pending further review feedback from a reviewer. * needs-reply The branch has been reviewed and awaits a response from the code/patch author. * merge-conditional The branch has been approved for merging pending comments or minor fixes that the review pointed out in his review message. * merge-approved The branch has been OK to be merged, unconditionally. * work-in-progress 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. 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. 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: {{{ .--------------------. | | v | .--------------. .-------------. START ---> | needs-review | ---> | needs-reply | '--------------' '-------------' /|\ .-------------------. / | \ .----------------. | merge-conditional | <--' | '--> | merge-approved | '-------------------' | '----------------' | .------------------. | work-in-progress | '------------------' }}} |
If your branch changes the UI in any significant manner (even text rewordings) you'll need to seek a [[UI/Reviews|UI review]]. The UI reviewers are listed on ReviewerSchedule under the ''specialties'' column. If your UI reviewer is being mentored you'll need to follow the procedure outlined above to ensure the UI review is properly reviewed. |
| Line 108: | Line 92: |
| If, after a successful review process, you submit your code to PQM only to find conflict you need to resolve the conflicts: | If, after a successful review process, you submit your code to PQM only to find conflict you need to resolve the conflicts: |
| Line 110: | Line 95: |
| 1. Merge rocketfuel onto your reviewed branch, resolve the conflict, push, and submit another merge request. | 1. Merge rocketfuel onto your reviewed branch, 1. resolve the conflict, 1. push, and 1. submit again to PQM. |
| Line 112: | Line 100: |
| 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. | 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. |
| Line 116: | Line 107: |
| 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: | You must include a commit message for PQM describing the change. If you use {{{ec2 land}}} or {{{bzr lp-land}}}, those commands will take care of adding necessary tags, using information in the merge proposal. |
| Line 118: | Line 109: |
| * If the modification went through the normal review process, you should add an {{{ r=fooReviewer }}} string to your merge summary. |
* {{{describe the change.}}}<<BR>> |
| Line 123: | Line 111: |
| * 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 }}} |
On the other hand, if you use something other than {{{ec2 land}}} or {{{bzr lp-land}}}, you'll need to manually include an item who the reviewer of the change was. This is the general form of a commit message: |
| Line 130: | Line 114: |
| * 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. |
* {{{[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 135: | Line 120: |
| * 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. |
* {{{[testfix]}}} is a special leading token required to land a branch when trunk branch is broken and future merges must supply a fix. |
| Line 142: | Line 126: |
| When you are ready to submit your branch to pqm for merging, use the following command (from RocketFuelSetup): | When you are ready to submit your branch to pqm for merging, use the following command (from RocketFuelSetup): |
| Line 148: | Line 133: |
| where {{{[r=reviewer]}}} is the most common case, but may be substituted with any review status from above as appropriate. Here's an example: | You can simplify the process of running tests on EC2 and landing your branch by doing the following: 1. Get your merge proposal reviewed and in the 'Approved' state, 1. Set the 'Commit Message' on the merge proposal to be the description of the branch, as above but without the reviewers and bug information. (Only the part above marked 'describe the change'.) 1. {{{% utilities/ec2 land}}} -- voilà, that's it! 1. Enjoy a snack, a nap, or take in a movie while ec2 runs the test suite and then submits your branch to PQM. |
| Line 150: | Line 143: |
| {{{ bzr pqm-submit -m'[r=BjornT] Fix bug 523.' }}} |
=== Community contributors === |
| Line 154: | Line 145: |
| 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: | Only Canonical employees are currently allowed to submit to the Launchpad PQM. Because we deploy code that passed PQM directly to a staging service, a patch could accidentally disclose confidential data and Canonical is not willing to risk that happening due to a non-staff change. As such non Canonical Launchpad developers need to find a Canonical Launchpad developer to test and land their changes. Your reviewer is expected to do it but do not assume he will -- explicitly ask him to do so when the review is approved. |
| Line 156: | Line 148: |
| {{{ 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. |
If your original reviewer is unavailable when it is time to land your branch, please go to #launchpad-dev and ask any reviewer there to land your approved branch for you. |
This page covers the process for effectively dealing with code and design reviews.
Contents
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 LaunchpadEnhancementProposalProcess 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.
Feature review
Consult the LaunchpadEnhancementProposalProcess and the UI design pages to get the workflow that should be completed before starting work on changes to the UI.
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. If you are a reviewer and choose not to have it reviewed by another reviewer, you can simply approve your merge proposal and land it at this point.
Once your reviewer has had time to go over the modifications, they will reply to the proposal with a description of areas or items that can be fixed, modified or improved in your code. They may also suggest better spellings of things in your code, or get into a discussion about domain specifics or obscure corner cases which they don't have enough personal knowledge of.
You must respond to each individual suggestion and question made by your reviewer; one of the main dangers with reviews is wasting everyones time by causing lots of back and forth discussions --- 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 review. If you want to make a case why a specific suggestion should not be followed then you must bring it up with the reviewer and not merge until agreement has been reached and documented in the merge proposal.
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.
Mentored reviews
When developers apply to become Launchpad reviewers they are assigned a mentor. We began referring to the person being mentored as a mentat, a made-up word you may see in reference to mentoring.
If your merge proposal is reviewed by a mentat his/her mentor will need to review the review. The mentat should add his mentor to the merge proposal. You must wait for both to approve the merge proposal before merging it.
UI reviews
If your branch changes the UI in any significant manner (even text rewordings) you'll need to seek a UI review. The UI reviewers are listed on ReviewerSchedule under the specialties column. If your UI reviewer is being mentored you'll need to follow the procedure outlined above to ensure the UI review is properly reviewed.
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:
- Merge rocketfuel onto your reviewed branch,
- resolve the conflict,
- push, and
- submit again to PQM.
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
You must include a commit message for PQM describing the change. If you use ec2 land or bzr lp-land, those commands will take care of adding necessary tags, using information in the merge proposal.
describe the change.
On the other hand, if you use something other than ec2 land or bzr lp-land, you'll need to manually include an item 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'
You can simplify the process of running tests on EC2 and landing your branch by doing the following:
- Get your merge proposal reviewed and in the 'Approved' state,
- Set the 'Commit Message' on the merge proposal to be the description of the branch, as above but without the reviewers and bug information. (Only the part above marked 'describe the change'.)
% utilities/ec2 land -- voilà, that's it!
- Enjoy a snack, a nap, or take in a movie while ec2 runs the test suite and then submits your branch to PQM.
Community contributors
Only Canonical employees are currently allowed to submit to the Launchpad PQM. Because we deploy code that passed PQM directly to a staging service, a patch could accidentally disclose confidential data and Canonical is not willing to risk that happening due to a non-staff change. As such non Canonical Launchpad developers need to find a Canonical Launchpad developer to test and land their changes. Your reviewer is expected to do it but do not assume he will -- explicitly ask him to do so when the review is approved.
If your original reviewer is unavailable when it is time to land your branch, please go to #launchpad-dev and ask any reviewer there to land your approved branch for you.
