Goal
- The main repository contains only codes that have been reviewed
- Reviewing should happen quickly
- It should integrate with our tools
- Developers shall be able to push items for review without having to pre-determine a reviewer
Considerations
- There are a lot of issues with maven branches, so they won't be used
- Introducing a review solution like review-board (http://review-board.org/) is too big a task to be done without being explicitly part of a sprint backlog.
Issues as Review Units
The unit of a review is a JIRA issue. If an issue is too big to be resolved in a day or less, it has to be splitted. If an issue is marked as resolved in JIRA, this means it's scheduled for review.
Steps to Tackle an Issue
- Identify the project whose issue requires an action upon (this can exceptionally be more than one project, but usually in this case new issues are created)
- If the version number in the pom does not end with "-SNAPSHOT", Snapshotize it as described below.
- Create new projects as needed and add the to the dependency management of the parent pom (in default).
- Clone the repository on the server, so that the repository's SSH-URI of the clone contains the identifier of the issue instead of 'default'.
- Act upon the cloned repository (i.e. clone it onto your local machine, and push as needed)
- Mark the issue as resolved after having pushed the final changes
- The reviewer will review the code and either close the issue and merge the issue down to default or write a review report in JIRA and reopen the issue
Variants
- The developer might want to post the test suite for review before starting the actual implementation, in this case an extra issue for the tests is opened in JIRA but no repository for that issue is created, instead the repository for the main issue is used (rationale: we must not commit failing tests to default).
The Review
The reviewer makes sure that:
- The code conforms to the coding guidelines
- Tests demonstrate the correct working of the code (in terms of the issue to be resolved)
- The code is a minimal solution solving the issue at hand
- The documentation is complete and concise
- The design is reasonably elegant (i.e. it doesn't stink, http://c2.com/cgi/wiki?CodeSmell)
- The code contains no duplication
- Typo in comments may be corrected directly; these changes are to be committed and pushed
If the code is accepted:
- If a project in the isse is new, the default branch is created and the project is added to continuum
- The code is pushed to default (script: tx-pushtodefault)
- If the code cannot be pushed because this would create multiple remote heads the reviewer may merge down changes from default instead of reopening (script: tx-updatefromdefault), the reviewer must make sure the merged code compiles, when merges are problematic or comprehensive the issue must be reopened for the developer to do them.
- The issue branches are removed (script: tx-removeissue)
- The isse is closed
Rules
- Never push your own code to default
- Review tasks have higher priority than implementation tasks
- Bulk-operations over multiple repositories (like changing version number and the like) do not undergo a review process, but should be done pairing or exceptionally with a post-commit review.
- The reviewer doesn't change the code, but proposes changes in the review report
- Code isn't ready for review if it doesn't compile or lacks tests or documentation
- Everybody may review but code written by somebody that is not in the core-team must be reviewed by a member of the core-team
- An issue may not be marked as resolved if it depends on an issue that isn't closed (i.e. while a review for a fix is pending, you may work on an issue that depends on the fix, but not mark it as resolved until the fix has been accepted)
Snapshotize
- If the version of P (in the pom of P) ends with -SNAPSHOT do nothing and return
- if the parent isn't already a SNAPSHOT-version (in the pom of the parent) make it a SNAPSHOT
version (increasing version) - change the version of P to a snapshot version and change the
parent to the latest parent-snapshot version (in the pom of P) - In the dependency management in the parent pom, change the version of
P to the newly set snapshot-version - for every project D depending on P, recurse at step 1 with P = D
Snapshotize script
A utility tool can be used to snapshotize a project
- check out all projects to a folder
- in that folder run txr snapshotize org.trialox.parent org.trialox.* <projecte-to-snapshotize>
Versioning
Versions are identified as follows
<primary version number>.<secondary version number>[.<tertiary version number>]. A tertiary version is omitted when it would be 0 and thus never occurs if the primary version number is 0.
A project starts with the version 0.1, so the first code committed is version 0.1-SNAPSHOT.
If the primary version number is 0 the version is implicitely beta. After that the names look like 1.0-rc-1, and 1.1-beta-1. [TODO specify more]
A version is increased as follows:
- If the primary version is 0 then the secondary version number is increased by one, e.g. 0.9 -> 0.10
- If the primary version is > 0 the tertiary version number is increased by one, e.g. 1.0.3 -> 1.0.4
- Increases of the the secondary version number for projects when the primary version number is > 0 do happen by decision of the customer, the same applies for changes of the primary version number.
Checking version consistency
For artifacts for which we specified a version in the dependency management we want all projects we maintain to depend on that version (his is why after changing a version in the dependency managemet we snapshotize all projects that use this dependency).
To check that all projects use the right version we have the a tool that can be invoked as follows:
java -jar org.trialox.internal.process/target/org.trialox.internal.process-0.1-SNAPSHOT-dist.dir/org.trialox.internal.process-0.1-SNAPSHOT.jar org.trialox.trialox *
This assumes that you are in a folder containing a built org.trialox.internal.process, the parent project and all (other) you ant to verify against the distribution management of the parent. The script will output warning:
- when a pom defines a project in a version different than the one in dependeca management -> fix either project or dependency management
- when a pom has a dependency to another version than the one in dependecy-management -> typically the pom is inheriting from an old parent, snatpshotize it
Comments (1)
Sep 19, 2008
Reto Bachmann says:
Code reviews at nokia (group and peerreviews):Code reviews at nokia (group and peer-reviews): http://www.gorkem-ercan.com/2008/09/how-do-we-do-code-reviews.html