ITK/Release 4/Modularization/Code Reviews/Process: Difference between revisions

From KitwarePublic
Jump to navigationJump to search
No edit summary
 
(12 intermediate revisions by 3 users not shown)
Line 2: Line 2:


__TOC__
__TOC__
= Potential Implementations =
# Create a Dedicated MANTIS instance for the project "ITKv4Review"
# Create a Dedicated Git repository for the project "ITKv4Review"
# Create a Dedicated Trac instance for the project "ITKv4Review"
In each one of these cases, we could adopt the review workflow described below


= Overview =
= Overview =


In this case we describe the process assuming a Git-based implementation.
A Git repository will be used to host the comments from reviewers.


* Kitware will create a separate Git repository containing
* Kitware will create a separate Git repository containing
Line 20: Line 12:
*** itkImage.h.txt  will be the code review file for itkImage.h
*** itkImage.h.txt  will be the code review file for itkImage.h
* The repository will contain at the top level, one directory per contractor, and under that directory we will replicate the monolithic directory tree structure of ITK with only the files that have been assigned to that contractor.
* The repository will contain at the top level, one directory per contractor, and under that directory we will replicate the monolithic directory tree structure of ITK with only the files that have been assigned to that contractor.
* This text files will be populated by the code reviewers with the following standardize content
* This text files will be populated by the code reviewers with comments describing their observations during the code review process.
 
= Workflow =
 
This section describes the life-cycle of a file when going through the code review process.


  STATUS: StatusKey (see table below)
Two concepts are introduced here:
 
  Arbitrary texts, as detailed as the reviewer deem necessary. it could be paragraphs.


= Workflow =
* Status: the current state of a given file in the code review process
* Rating: the severity of the modifications needed by this file


== Graphical Overview ==
== Graphical Overview ==
Line 45: Line 40:
== Review Status ==
== Review Status ==


For each file, a contractor will review the code following the Check list (reference here) and specify one of the following potential status
For each file, a contractor will review the code following the [[ITK_Release_4/Modularization/Code Reviews/Checklist|Checklist]] and specify one of the following potential status


* '''Pending''': No review has been performed yet.
* '''Pending''': No review has been performed yet.
Line 54: Line 49:


* All files start in the '''Pending''' state.
* All files start in the '''Pending''' state.
* From '''Pending''' to '''Rated''': A reviewer will review the file following the CheckList (reference here), and identify actions to be taken, and will assign a developer to look into the details. The reviewer will assign a '''rating''' according to the list below.
* From '''Pending''' to '''Rated''': A reviewer will review the file following the [[ITK_Release_4/Modularization/Code Reviews/Checklist|Checklist]], and identify actions to be taken, and will assign a developer to look into the details. The reviewer will assign a '''rating''' according to the list below.
* From '''Rated''' to '''Done''': The assigned developer will communicate back to the reviewer when the actions have been taken, the reviewer will verify the changes and, if satisfied, will set the status to '''Done'''.
* From '''Rated''' to '''Done''': The assigned developer will communicate back to the reviewer when the actions have been taken, the reviewer will verify the changes and, if satisfied, will set the status to '''Done'''.
* From '''Pending''' to '''Done''': A reviewer may make this transition if the rating is '''Good''' as defined in the rating section below.
* From '''Pending''' to '''Done''': A reviewer may make this transition if the rating is '''Good''' as defined in the rating section below.
Line 62: Line 57:
* '''Good''': The file has been reviewed, and no change was required.
* '''Good''': The file has been reviewed, and no change was required.
* '''Minor''': Minor typos. In this case the reviewer will fix the problems directly (and still submit them to Gerrit). Minor changes are defined a those that do not influence the API. (including doxygen documentation).
* '''Minor''': Minor typos. In this case the reviewer will fix the problems directly (and still submit them to Gerrit). Minor changes are defined a those that do not influence the API. (including doxygen documentation).
* '''Major''': File needs refactoring or major bug fixing. The reviewer must log a mantis and include the Bug number in the comments in the .txt review file, and the developer to which the bug has been assigned.
* '''Major''': File needs refactoring or major bug fixing. The reviewer must log a JIRA issue and include the issue number in the comments in the .txt review file, and the developer to which the bug has been assigned.
* '''Remove''': File must be removed (broke, obsolete, irreparable).
* '''Remove''': File must be removed (broke, obsolete, irreparable).


== Refactoring ==
== Refactoring ==


For classes to be refactored
For files to be refactored, that is, files that were rated as:


the following process should be followed
* Major
* Remove


* MANTIS Bug
the following process should be followed.
* Dependencies
* Migration guide


= Potential Implementations Details =
* The reviewer will log a JIRA Bug (in the standard ITK Bug tracker)
* The reviewer will assign this bug to the developer who may be more knowledgeable about the file, e.g. the first committer of the file).
* From there, the bug will be managed in the standard way.
** Dependencies with other changes will be discussed at this point
** The assigned developer will discuss fixing alternatives
** Will implement one of such alternatives
** The developer will push a patch to Gerrit
** Any API issues will be managed following the Migration guide documentation procedure
** The patch will go through code review
** Eventually will be merged into master
* Once the fix is merged, the bug will be closed, and the developer will notify the reviewer
* The reviewer will mark the issue as "Done" (using the adopted implementation: Git, JIRA or Trac).


== MANTIS ==
= Implementation Procedure =


The following describes the potential implementation of this workflow by using MANTIS as a tool.
== Git-Based ==


* A separate MANTIS instance will be created, specifically for ITKv4 Review
  https://github.com/InsightSoftwareConsortium/itk-retroactive-review
* The Status field will be created (Pending, Rated, Done)
* The Rating field will be created (Good, Minor, Major, Remove)
* A MANTIS issue will be created for every single file (done by a script)
** The reporter will be the contractor (done by a script)
** The reviewer will set the Status and Rating fields according to her review.
** Some of these issues may have to be mapped to the MANTIS issues in the standard ITK bug tracker


== Git ==
We will use a parallel Git repository for hosting the content of the reviews in free format text files.


The following describes the potential implementation of this workflow by using Git as a tool.
* The repository has a mirror copy of the Modular ITK directory tree structure.
 
* For each ITK source code file, this Git repository contains a text file.
* A separate Git repository will be created (re-using the existing accounts), specifically for ITKv4 Review
** The filename matches one-to-one the filenames in ITK, plus the extension .txt.
* The repository pre-populated (via scripts)
** It will have one directory per contractor
** Inside that directory the ITK directory structure (monolithic) will be replicated
** Inside those subdirectories, the files that are assigned to that contractor will be copied
** The files will be text files whose name matches one-to-one the filenames in ITK, plus the extension .txt.
*** For example, there will be a file  itkImage.h.txt for the reviews of the file itkImage.h.
*** For example, there will be a file  itkImage.h.txt for the reviews of the file itkImage.h.
* The presence of the empty .txt files will indicate a '''Pending''' status
* The presence of the empty .txt files will indicate a '''Pending''' status
* Reviewers will edit those files, as they review the ITK code, and will populate the files with the ratings and any additional comments that they consider relevant. The changes will be committed, and merged to the master branch of this dedicated repository.
* Reviewers will edit those files, as they review the ITK code, and will populate the files with the ratings and any additional comments that they consider relevant. The changes will be committed, and merged to the master branch of this dedicated repository.
== Trac ==

Latest revision as of 16:00, 9 December 2011

This page summarizes a process proposal for managing the code reviews of the modules in ITK.

Overview

A Git repository will be used to host the comments from reviewers.

  • Kitware will create a separate Git repository containing
    • One text file for each one of the files in the current ITK Manifest.
    • The files will be named after the ITK files. For example
      • itkImage.h.txt will be the code review file for itkImage.h
  • The repository will contain at the top level, one directory per contractor, and under that directory we will replicate the monolithic directory tree structure of ITK with only the files that have been assigned to that contractor.
  • This text files will be populated by the code reviewers with comments describing their observations during the code review process.

Workflow

This section describes the life-cycle of a file when going through the code review process.

Two concepts are introduced here:

  • Status: the current state of a given file in the code review process
  • Rating: the severity of the modifications needed by this file

Graphical Overview

This is a graph with borders and nodes. Maybe there is an Imagemap used so the nodes may be linking to some Pages.

Review Status

For each file, a contractor will review the code following the Checklist and specify one of the following potential status

  • Pending: No review has been performed yet.
  • Rated: Review has been performed and it resulted into actions to be performed by a developer.
  • Done: Corrective actions have been taken. No further action required.

Transition Rules

  • All files start in the Pending state.
  • From Pending to Rated: A reviewer will review the file following the Checklist, and identify actions to be taken, and will assign a developer to look into the details. The reviewer will assign a rating according to the list below.
  • From Rated to Done: The assigned developer will communicate back to the reviewer when the actions have been taken, the reviewer will verify the changes and, if satisfied, will set the status to Done.
  • From Pending to Done: A reviewer may make this transition if the rating is Good as defined in the rating section below.

Rating

  • Good: The file has been reviewed, and no change was required.
  • Minor: Minor typos. In this case the reviewer will fix the problems directly (and still submit them to Gerrit). Minor changes are defined a those that do not influence the API. (including doxygen documentation).
  • Major: File needs refactoring or major bug fixing. The reviewer must log a JIRA issue and include the issue number in the comments in the .txt review file, and the developer to which the bug has been assigned.
  • Remove: File must be removed (broke, obsolete, irreparable).

Refactoring

For files to be refactored, that is, files that were rated as:

  • Major
  • Remove

the following process should be followed.

  • The reviewer will log a JIRA Bug (in the standard ITK Bug tracker)
  • The reviewer will assign this bug to the developer who may be more knowledgeable about the file, e.g. the first committer of the file).
  • From there, the bug will be managed in the standard way.
    • Dependencies with other changes will be discussed at this point
    • The assigned developer will discuss fixing alternatives
    • Will implement one of such alternatives
    • The developer will push a patch to Gerrit
    • Any API issues will be managed following the Migration guide documentation procedure
    • The patch will go through code review
    • Eventually will be merged into master
  • Once the fix is merged, the bug will be closed, and the developer will notify the reviewer
  • The reviewer will mark the issue as "Done" (using the adopted implementation: Git, JIRA or Trac).

Implementation Procedure

Git-Based

  https://github.com/InsightSoftwareConsortium/itk-retroactive-review

We will use a parallel Git repository for hosting the content of the reviews in free format text files.

  • The repository has a mirror copy of the Modular ITK directory tree structure.
  • For each ITK source code file, this Git repository contains a text file.
    • The filename matches one-to-one the filenames in ITK, plus the extension .txt.
      • For example, there will be a file itkImage.h.txt for the reviews of the file itkImage.h.
  • The presence of the empty .txt files will indicate a Pending status
  • Reviewers will edit those files, as they review the ITK code, and will populate the files with the ratings and any additional comments that they consider relevant. The changes will be committed, and merged to the master branch of this dedicated repository.
Retrieved from "https://public.kitware.com/Wiki/index.php?title=ITK/Release_4/Modularization/Code_Reviews/Process&oldid=44231"