launchpad developmentName: Optional Reviews
Owner: Technical Architect
Effective: Feb 2011
Review: Epic Jan 2011
Experiment Underway:
Process Overview
Some reviews are not needed and eligible developers can choose to have branches land without formal review.
This process is now part of the primary launchpad code review process. The experiment page is being kept around while we decide what to do with UI reviews as far as self-review goes.
Process Description
Launchpad reviewers can choose to land branches without review. Only 'code' reviews can be made optional. Javascript UI is out of scope of the experiment.
For the first two weeks (until 12 Nov 2010) all unreviewed landings will be reviewed post landing by Robert.
Each month, all code reviewers will be assigned one unreviewed landed to do a post hoc review as part of assessing the experiment.
Production problems which are tracked to unreviewed landings will also be input into assessing the experiment.
Rationale
Not all changes benefit from code review / are high enough risk to need formal review. For instance (not exhaustive):
- mechanical things (like moving code)
- updating source deps
- rollbacks
- typo fixes
- improvements to documentation
For many of these things a review may add value - but less value than doing the review uses up. We want reviews to be a net win for the effort being put into developing Launchpad, and so we should, once we're comfortable people know how things should be, allow them to decide if a particular change is an improvement on its own, or an improvement that also /needs/ other input before landing.
Triggers
Experienced developer (someone currently working on LP who has been doing so for the last 3 months) who is also a reviewer of the sort of change they want to land, landing a code change (not UI, for now : evaluate after assessing the success of optional code reviews).
Activities
Submit the branch to create an MP (our toolchains can look at this and it provides a location for a post landing review if the branch has that done to it.)
Self review without a review type.
- Land via the normal landing process.
Postreviewed landings (week 4) 11957 to ...
revno |
branch |
reviewer |
would-review-be-worth-it |
comments |
11959 |
devel |
lifeless |
No |
Tweak log levels to eliminate some unneeded OOPSes. |
11985 |
devel |
flacoste |
No |
Subscribing to tags has performance problems, reverting for now |
11991 |
devel |
bigjools |
Yes |
Commit build status changes to database before moving the upload directory. - review caught a missing test case |
11993 |
devel |
mars |
No |
Turn off _setAllowHash in Google Analytics configuration to track visitors across subdomains |
12014 |
devel |
flacoste |
No |
Land buildd-manager changes on devel that are already landed on db-devel by mistake. |
12015 |
devel |
jelmer |
No |
Make sure that we only publish the "main" component for PPA indexes. Currently it processes all components unnecessarily which wastes a lot of time. |
10012 |
db-devel |
flacoste |
No |
Fix database patch number |
10017 |
db-devel |
flacoste |
No |
Revert database patch application timing report, which isn't working as expected when patches are all applied in a single transaction. |
10034 |
db-devel |
thumper |
No |
Don't apply comments.sql in the replicated environment |
Postreviewed landings (week 3) 11927 to 11956
11955 |
devel |
lifeless |
Yes |
Delete a test which was broken by a bad landing, but actually has (unclear) value. |
11954 |
devel |
lifeless |
No |
Disable a spuriously failing test. |
11947 |
devel |
lifeless |
No |
Reduce oopses by fixing log levels. |
11944 |
devel |
lifeless |
No |
Fix landing where concurrent patches interacted. |
11940 |
devel |
lifeless |
No |
Code in live use copied back into the source tree. |
11936 |
devel |
jml |
No |
Apply fix prepped by stub for Person:+commentedbugs timeouts. |
11930 |
devel |
lifeless |
No |
Revert 11917 now its not needed. |
Postreviewed landings (week 2) 11876 to 11926
11917 |
devel |
lifeless |
No |
Fix the fallout from 11905 for staging. |
11914 |
devel |
jml |
No |
Fixes prior bad landing that passed review. |
11906 |
devel |
lifeless |
No |
Testfix a bad landing. |
11905 |
devel |
lifeless |
No |
Apocalypse progress - broke staging, but review wouldn't have caught it. |
11893 |
devel |
lifeless |
No |
Fix a missing DB permission showing up in OOPS. |
11887 |
devel |
lifeless |
No |
Bump a dependency version, cleanup code as a result. |
11877 |
devel |
jml |
No |
Bump a dependency version. |
11876 |
devel |
lifeless |
No |
Test improvement, was mailed to the list. |
Postreviewed landings (week 1) 11823 to 11875
revno |
branch |
reviewer |
would-review-be-worth-it |
comments |
11874 |
devel |
lifeless |
No |
Updates a dependency. |
11855 |
devel |
lifeless |
No |
Clear fix with no alternative implementations. |
11851 |
devel |
lifeless |
Yes |
Added a edit-subscription facility for existing subscriptions - review caught a mismatch between subscribe and edit-subscription models. |
11842 |
devel |
lifeless |
No |
Fixed up a XXX landed while a dependent layer was fixed. |
11836 |
devel |
lifeless |
No |
Simple drop of unneeded dep combined with a testfix to trigger buildbot |
11827 |
devel |
lifeless |
No |
Totally mechanical, restructure along already agreed lines |
11823 |
devel |
lifeless |
No |
A testfix - fixing something landed without ec2land |
