Continuous Delivery and Code Reviews

[Frederic Conrotte]

Hello

After reading the Continuous Delivery book, If I'm not wrong it does
not talk about code review.

I'm working on a team of around 15 developers with too much juniors
for the moment.

We have a basic Continuous delivery pipeline implemented but now we
would like to perform systematic code review to improve code quality.

Taking figure 5.4 on page 111 of the book, at which stage of the
pipeline do you think code review should be performed?

My feeling is:
- All developers should commit on a separate trunk named "to_review"
- the team leader performs code reviews and patch the main trunk with
successfully reviewed commits.
- We should create a new "code review" stage between "Commit stage"
and "acceptance stage".

What's your opinion ? Does it make sense ?

[Larry Cai]

Hi:

My suggestion for this is to take a look at git/gerrit, it serves well for your situation.

- delivery is done in master branch

- all codes are done in review branch

- if the codes are passed review and verification (like acceptance testing), they can be allowed to merge to master branch

the gerrit tool support those things so smoothly that our team adapted it quickly.

rgs/larry

--
True software development embraces consistent inconsistency.
blog: http://codeslife.com

[Kief Morris]

Two words: pair programming. Far more effective than reviews done later, and avoids adding bottlenecks to your process.

[Adam Rosien]

Also, "hey, come over here and look at this code I just wrote", if you're not regularly doing pair programming.

[Tan Yeong Sheng]

Ideally if you are practising scrum with pair programming, the code reviews are done continually within the pair.

[Jez Humble]

I am currently writing a blog post on this topic. The short answer is
this:

* The best way to review code is through pair programming
* It's a bad idea to gate merge to mainline - by creating a separate
branch, for example - on a formal review process. This inhibits
continuous integration (the best way of reducing the risk of bad
changes, which is what you are really aiming to achieve).
* I think Gerrit is a nice tool, but it should be used *after* check-
in (that's how it's designed, in fact). Part of the job of the senior
developers is to review all check-ins. They could, for example,
subscribe to a feed.

To summarize: code review is good. So good, we should be doing it
continuously, through pair programming and reviewing commits. If a
senior dev finds a bad commit, she should pair with the person who
committed it to help them fix the problem.

Gating merge to mainline on a formal review is bad, and creating
branches to do so is extra bad, for the same reason that feature
branches are bad.

Thanks,

Jez.

On Feb 5, 2:30 am, Frederic Conrotte <frederic.conro...@gmail.com>
wrote:

[Frederic Conrotte]

Many thanks to all for your advices. We are already doing some pair programming code review but I wanted a more systematic process.

I guess systematic code reviews does not fit well with Continuous Deployment.

With this methodology people should rather review x% of commits where in practice 10 < x < 30

2012/2/11 Jez Humble <jhu...@jezhumble.net>

[Adam Rosien]

I'm not clear what you want as a result of a more systematic process. I assume you mean you want to have higher quality code.

There is no magic formula for the correct % of code that needs to be reviewed. If you are measuring too many bugs per release, why do you think are you having so many bugs? You need to answer that question. Are your tests not catching things? Are your developers less experienced and need more training?

IMHO code reviews are a sign that the reviewer doesn't trust the coder, so it is the organization's job to increase trust in the developer, through education (pair programming, code reviews, etc.) until you trust them. And trust needs to be measured by something like how many bugs a person introduces. Trust removes process, removing process speeds up development.

.. Adam

[Jez Humble]

Hi Frederic

I definitely advocate a systematic approach. All check-ins should be systematically reviewed by the dev lead as soon as possible after the check-ins occur. Gerrit is a nice tool for this.

Furthermore, all code that is to be checked in must have been reviewed by one other person before check-in (as part of pair programming). This should also be systematic, i.e. built into the system.

So 100% of commits should be reviewed, both before and after check-in (i.e. twice).

What I don't recommend is creating gated processes that try to enforce this, either by using branches in version control, or by adding stages to the pipeline. I say this for two reasons:

1. it inhibits continuous integration

2. it assumes that people are going to be stupid and that your process must correct for this, rather than that people will overall try to do the right thing, but that we must put something in place to manage the exceptions. Basically, your process should follow theory Y not theory X: http://en.wikipedia.org/wiki/Theory_X_and_theory_Y

In general, managers spend too much time trying to create complex processes to try and enforce "good behaviour" rather than building systems in which good behaviour naturally emerges. I like to call this kind of Theory X thinking "risk management theatre" because it provides the impression that risk is being managed effectively, while actually producing lower quality results.

Thanks,

Jez.

--
Jez Humble

Co-author, Continuous Delivery

http://continuousdelivery.com/

http://jezhumble.net/

[Patrick Bossman]

What do you see people doing wrt database schema changes here?

My preference is deploy the change to a separate schema to do automated checks and tests, then present the DBA with the schema change, deployment process change, and results of the tests for a one time review/approval before a merge.

Patrick Bossman

[David Farley]

Patrick,

     I generally prefer to omit the "approval" step. I will use automated testing, deployment and data-migration as part of my deployment strategy and will rely on pair-programming to provide the review. 

If, as a developer I feel the need for a more expert review of some particularly complicated change, I may ask someone with the appropriate experience to take a look while I am working on it. 

In general I think that Continuous Delivery works best as a "Flow" process. We want ideas to flow through the development process and into the hands of our users. 

I prefer that all my gates are automated. I do value human decision making, but I want that to act as part of this flow-process. I prefer to "build quality in" to my products rather than try to "inspect quality in" after the fact which is, to my mind, what most sign-off or gating-processes tend to do.

   Dave Farley