How do we use Review Board

How do we use Review Board


We are quite conservative in the use of a version control system. Three years ago, Vadym had convinced us to start using SVN instead of CVS. At the same time Nerrvana came into our lives and we wanted to try some new systems that we thought would improve the process of its development. So we eventually adopted PHP Under Control (PUC) and Review Board (RB).

Today we will talk about RB. We use RB in the post commit mode. In this mode code is first committed to the SVN repository, and then later SVN RB hook* picks it up, adds to the RB which in turn sends letters to all the participants signed on changes in this SVN repository.

There is another mode – pre-commit. More information can be found in the RB documentation. This mode requires that the review request is created by hand. We thought it would be much easier to get it to do SVN. What if the errors will slip into the repository? They get there anyway :) But the requests are generated automatically, and errors in the code can be fixed in the next commit.

We have several repositories for different projects. RB makes it easy to customize who will get review requests. First, if there is no hook in the repository, there will be no review requests. Second, as shown in the screenshot below, within the administrative interface RB has a page that allows you to fine tune who gets notified with regular expressions.

Fig. 1 Fine tuning who gets what in admin UI

How does it work for us?

Let’s take a project were three of us are involved – me, Dmitry and Alexander. For example I did a commit. Immediately after a commit, a review request appears in RB with Open status. All three of us are working on this project will get a message about a new review request.

click to enlarge

Fig. 2 Dashboard
(click to enlarge)

Using the link inside an email Dmitry and Alex can open diff, and see if everything is all right. We agreed that if all goes well, we review by setting the flag “Ship it!” You can also write a comment like “OMG – so much work, congrats on finishing” or “Have no idea what you did, life will prove if you are right. The first three lines – OK”.

click to enlarge

Fig. 3 Review Request
(click to enlarge)

Let’s say, Alex looked at my commit in RB and is happy with everything. He marks the review as “Ship it” straight away. However Dmitry has some comments. He writes about them, and leaves the review as is (not marking it “Ship it”). All of us will get an email from RB with Dmitry’s comments. I am reading it, looking at the code and writing a response inside Review Board precisely around line 214 where Dmitry commented. For example – ‘Thank you, my terrible mistake – do not tell my mum please’.

Fig. 4 Ship it

Then I correct an error in the code and make a new commit with the commit message containing a link to this review. This creates yet another review request, but by looking at its commit message, containing a reference to RB review, it becomes clear to all that this commit fixes up a problem as a result of previous review. After that, Dmitry with a clear conscience, marks both reviews (one where problem was found and one where it was corrected) as the “Ship it”. It is quite possible there were no bugs in the code. In this case I write an explanation in the comments within the code directly in RB. Dmitry responds to it.

Fig. 5 Review menu

Alex can also join the discussion around that piece of code inside review request. In the end, Dmitry marks a review as “Ship it”, making it clear to me that it can be closed now. If I see two “Ship it” flags on my review request – this means both Dmitry and Alex are happy with a change, and close review request (Close-> Submitted). It should be noted that with each new commit or each new comment RB sends mail to all of us with links. Here is the message sent by RB to me with a message from Alex that he looked at my changes and marked it “Ship it”.

Fig. 6 Email notification

Once you leave a comment you’ll still get an email. An email about your own activity. We did not find how to configure the RB so that the author did not receive mail on their own activity and this is perhaps the only thing missing in RB. However, this does not affect our RB perception, which is very good overall.

In conclusion we are totally happy with Review Board and highly recommended it. If there are issues with the setup –  contact, we’ll help with what we can.

P.S. I almost forgot – here is RB of RB development team. You may want to look at it.

Fig. 7 Inline comments on RB site


* – about hooks
We took the hook for SVN. Vadym changed it by commenting out things we do not need and connected to SVN. You can read more about the hook for SVN here.

Here’s the code we use:

#!/bin/bash
REPOS="$1"
REV="$2"
cd ~apache
/usr/bin/python /var/svn/hooks/reviewboard-post-commit-hook.py "$REPOS" "$REV" &>/dev/null || exit 1

This is an actual file reviewboard-post-commit-hook.py we use:

View Code PYTHON
#!/usr/bin/env python
#
# reviewboard-post-commit-hook
# This script should be invoked from the subversion post-commit hook like this:
#
# REPOS="$1"
# REV="$2"
# /usr/bin/python /some/path/reviewboard-post-commit-hook.py "$REPOS" "$REV" || exit 1
#
# Searches the commit message for text in the form of:
#   publish review - publishes a review request
#   draft review - creates a draft review request
#
# The space before 'review' may be ommitted.
#
# The log message is interpreted for review request parameters:
#    summary = up to first period+space, first new-line, or 250 chars
#    description = entire log message
#    existing review updated if log message includes 'update review:[0-9]+'
#    bugs added to review if log message includes commands as defined in
#      supported_ticket_cmds
#
# By default, the review request is created out of a diff between the current
# revision (M) and the previous revision (M-1).
#
# To create a diff that spans multiple revisions, include
# 'after revision:[0-9]+' in the log message.
#
# To limit the diff to changes in a certain path (e.g. a branch), include
# 'base path:"
"' in the log message.  The path must be relative to
# the root of the repository and be surrounded by single or double quotes.
#
# An example commit message is:
#
#    Changed blah and foo to do this or that.  Publish review ticket:1
#      update review:2 after revision:3 base path:'internal/trunk/style'.
#
# This would update the existing review 2 with a diff of changes to files under
# the style directory between this commit and revision 3.  It would place
# the entire log message in the review summary and description, and put
# bug id 1 in the bugs field.
#
# This script may only be run from outside a working copy.
#
 
#
# User configurable variables
#
 
# Path to post-review script
POSTREVIEW_PATH = "/usr/bin/"
# Username and password for Review Board user that will be connecting
# to create all review requests.  This user must have 'submit as'
# privileges, since it will submit requests in the name of svn committers.
USERNAME = 'XXXXX'
PASSWORD = 'XXXXXXXXXXX'
 
# If true, runs post-review in debug mode and outputs its diff
DEBUG = False
 
#
# end user configurable variables
#
 
import sys
import os
import subprocess
import re
import svn.fs
import svn.core
import svn.repos
 
# list of trac commands from trac-post-commit-hook.py.
# numbers following these commands will be added to the bugs
# field of the review request.
# supported_ticket_cmds = {'review':         '_cmdReview',
#                          'publishreview':  '_cmdReview',
#                          'publish review': '_cmdReview',
#                          'draftreview':    '_cmdReview',
#                          'draft review':   '_cmdReview'}
#
# ticket_prefix = '(?:#|(?:ticket|issue|bug)[: ]?)'
# ticket_reference = ticket_prefix + '[0-9]+'
# ticket_command = (r'(?P[A-Za-z]*).?'
#                   '(?P%s(?:(?:[, &]*|[ ]?and[ ]?)%s)*)' %
#                   (ticket_reference, ticket_reference))
 
def execute(command, env=None, ignore_errors=False):
    """
    Utility function to execute a command and return the output.
    Derived from Review Board's post-review script.
    """
    if env:
        env.update(os.environ)
    else:
        env = os.environ
 
    p = subprocess.Popen(command,
                         stdin = subprocess.PIPE,
                         stdout = subprocess.PIPE,
                         stderr = subprocess.STDOUT,
                         shell = False,
                         close_fds = sys.platform.startswith('win'),
                         universal_newlines = True,
                         env = env)
    data = p.stdout.read()
    rc = p.wait()
    if rc and not ignore_errors:
        sys.stderr.write('Failed to execute command: %s\n%s\n' % (command, data))
        sys.exit(1)
 
    return data
 
def main():
    if len(sys.argv) != 3:
        sys.stderr.write('Usage: %s  \n' % sys.argv[0])
        sys.exit(1)
 
    repos = sys.argv[1]
    reposname = os.path.basename(repos)
    rev = sys.argv[2]
 
    # verify that rev parameter is an int
    try:
        int(rev)
    except ValueError:
        sys.stderr.write("Parameter  must be an int, was given %s\n" % rev)
        sys.exit(1)
 
    # get the svn file system object
    fs_ptr = svn.repos.svn_repos_fs(svn.repos.svn_repos_open(
            svn.core.svn_path_canonicalize(repos)))
 
    # get the log message
    log = svn.fs.svn_fs_revision_prop(fs_ptr, int(rev),
                                    svn.core.SVN_PROP_REVISION_LOG)
 
    # error if log message is blank
    if len(log.strip()) < 1:
        sys.stderr.write("Log message is empty, no review request created\n")
        sys.exit(1)
 
    # get the author
    author = svn.fs.svn_fs_revision_prop(fs_ptr, int(rev),
                                       svn.core.SVN_PROP_REVISION_AUTHOR)
 
    # error if author is blank
    if len(author.strip()) < 1:
        sys.stderr.write("Author is blank, no review request created\n")
        sys.exit(1)
 
    # check whether to create a review, based on presence of word
    # 'review' with prefix
    # review = r'(?:publish|draft)(?: )?review'
    # if not re.search(review, log, re.M | re.I):
    #     print 'No review requested'
    #     sys.exit(0)
 
    # check for update to existing review
    # m = re.search(r'update(?: )?review:([0-9]+)', log, re.M | re.I)
    # if m:
    #     reviewid = '--review-request-id=' + m.group(1)
    # else:
    #     reviewid = ''
 
    # check whether to publish or leave review as draft
    # if re.search(r'draft(?: )?review', log, re.M | re.I):
    #     publish = ''
    # else:
    publish = '-p'
 
    # get previous revision number -- either 1 prior, or
    # user-specified number
    #m = re.search(r'after(?: )?revision:([0-9]+)', log, re.M | re.I)
    #if m:
    #    prevrev = m.group(1)
    #else:
    prevrev = int(rev) - 1
 
    # check for an explicitly-provided base path (must be contained
    # within quotes)
    #m = re.search(r'base ?path:([\'"][^\'"]+[\'"])', log, re.M | re.I)
    #if m:
    #    base_path = m.group(1)
    #else:
    #    base_path = ''
 
    # get bug numbers referenced in this log message
    # ticket_command_re = re.compile(ticket_command)
    # ticket_re = re.compile(ticket_prefix + '([0-9]+)')
 
    # ticket_ids = []
    # ticket_cmd_groups = ticket_command_re.findall(log)
    # for cmd, tkts in ticket_cmd_groups:
    #     funcname = supported_ticket_cmds.get(cmd.lower(), '')
    #     if funcname:
    #         for tkt_id in ticket_re.findall(tkts):
    #             ticket_ids.append(tkt_id)
 
    # if ticket_ids:
    #     bugs = '--bugs-closed=' + ','.join(ticket_ids)
    # else:
    #     bugs = ''
 
    # summary is log up to first period+space / first new line / first 250 chars
    # (whichever comes first)
    summary = '--summary=' + log[:250].splitlines().pop(0).split('. ').pop(0)
 
    # other parameters for postreview
    # repository_url  = '--repository-url=file://' + repos
    repository_url  = '--repository-url=http://XXXXXXXXXXX.com/svn/' + reposname
    password        = '--password=' + PASSWORD
    username        = '--username=' + USERNAME
    description     = "--description=%s[%s]: %s" % (reposname, rev, log)
    submitas        = '--submit-as=' + author
    revision        = '--revision-range=%s:%s' % (prevrev, rev)
 
    # common arguments
    args = [repository_url, username, password, publish,
            submitas, revision]
 
    # if not updating an existing review, add extra arguments
    #if len(reviewid) == 0:
    args += [summary, description]
 
    if DEBUG:
        args += ['-d', '--output-diff']
        print [os.path.join(POSTREVIEW_PATH, 'post-review')] + args
 
    # Run Review Board post-review script
    data = execute([os.path.join(POSTREVIEW_PATH, 'post-review')] + args,
                   env = {'LANG': 'en_US.UTF-8'})
 
    if DEBUG:
        print data
 
if __name__ == '__main__':
    main()
Print this post | Home

14 comments

  1. mattgenton says:

    hi to all at http://www.deepshiftlabs.com i thought i had sent this newyears eve but it didnt send so i have sent it again good luck for 2012 to you all
    – matt-gent

  2. Igor Kryltsov says:

    Hi Matt,

    Best wishes from all of us to you too!
    We are launching two new and completely different products in 2012. Stay tuned :)

    Cheers,
    Igor

  3. Andrey says:

    Доброго времени суток. Я внедряю ReviewBoard у себя в компании. Столкнулся с неприятным моментом, что review requests видны незалогиненым пользователям. У вас тоже такая ситуация или вы как-то обошли ее? Спасибо.

  4. Igor Kryltsov says:

    Мы используем сейчас 1.6.13 (не последнюю http://demo.reviewboard.org/account/login/?next_page=/dashboard/). На вкладке http://your.board.com/admin/settings/authentication/ “Authentication Method” выбран “Standard Authorization” и всё остальное НЕ заполнено. Если я захожу на http://your.board.com/account/login/, то я вижу только форму логина и ничего более.

  5. Andrey says:

    Большое спасибо. Причина была во флаге Enable Registration.

  6. Kirill Kolesnikov says:

    Доброго времени суток. Мы сейчас внедряем review board в команде. Продукт локализован в России. Подскажите вам удалось как нибудь победить проблему с кодировками? Дело в том, что “из коробки” при использовании все русские буквы после сохранения отображаются как “?????”. У вас возникали подобные проблемы?
    Спасибо.

  7. Igor Kryltsov says:

    Столкнулись конечно, но поскольку комментарии в коде мы пишем на английском, да и в самом коде нет русского языка, то мы просто решили использовать английский.

  8. Kirill Kolesnikov says:

    Решил проблему с кодировкой. Если кому интересно, перед созданием нового ревью сайта, создал базу на PostgreSQL с нужной кодировкой (Unicode). И на всякий случай поменял шаблоны в PostreSQL на Unicode по умолчанию.

  9. Igor Kryltsov says:

    I was thinking it is this:

    http://www.deepshiftlabs.com/dev_blog/wp-content/uploads/2013/01/2013-01-28_222338.png

    Update: No this does not help

  10. Bharani Prasad says:

    Hi,

    Can any one share me and working pre-commit script to integrate ReviewBoard with SVN.

    I want to implement this process of Code Review in my project.

    Please do suggest me some ideas and also on implementation, If one can share me the pre-commit script is will be really appreciated

  11. Andrey says:

    Bharani,

    No script is needed for integration with SVN. You just upload a diff file to ReviewBoard.

    dankevicha (at) gmail (dot) com

  12. ekalip says:

    Вдогонку про поддержку русского языка — если вы изначально создали базу не в UTF-8, не стоит тратить время на смену кодировок задним числом, все равно борда будет не довольна и успешно сохранит весь русский текст в виде ????? ??. Проще запустить мастер и подсунуть ему уже UTF-8.

  13. vreddy says:

    even I need pre-commit script , please share it I installed reviewBoard 1.7

  14. Igor Kryltsov says:

    We do not use pre commit hooks. Only post commit. We also run 1.7.14 now. I suggest to ask your question – https://groups.google.com/forum/?fromgroups#!forum/reviewboard