Agile Peer Review

Preface

What follows is primarily the result of nearly 2 years of intentional refinement by an experienced, high-performing agile team. Dev team members, architects, product owner, and even members of other dev teams participated and helped shape our thoughts on review. I also incorporated ideas from several other experienced peers. But like anything agile, it's a work in progress.

Don't let preconceptions about the term "review" color your reading - most of the reviews called for are only 5-15 minutes long.

Overview

Functional defects and design problems get exponentially more expensive the longer they are allowed to exist. The earlier those problems can be found and fixed, the better. Peer reviews provide an inexpensive, effective method for doing that.

There are several major work products in the lifecycle of a user story that warrant review (these are discussed in detail below):

• Domain Model - The team’s shared, fundamental, documented understanding of the domain.
• Demo script - How the dev team will demonstrate to stakeholders that the story is functionally complete.
• Design - Focused on the API and its test cases, but also covering important implementation details. 
• Pull Request - A single coherent change to the shared codebase, in service of a story. 
• Code Complete - The full set of changes to the codebase that were made to complete the story.

Benefits

These reviews deliver tangible value to the business

• Higher-quality code 
• Higher, more consistent velocity

And additional value to the dev team

• Pride in work 
• Self-improvement

As a result of the direct effects of the reviews

• Getting a better product, sooner 
• Bugs identified, eliminated earlier in the process 
• More maintainable code
• Better tested
• More adherent to team/organizational standards
• Knowledge sharing
• Team members sharing the same frame of mind
• Better understanding of the domain
• Better understanding of how system components interact
• Improving our craftsmanship every day by learning from each others strengths
• Better understanding of our tools and technical stack

What to Review

Domain Model

The fundamental, documented understanding of the domain shared among the whole team including the Product Owner. A critical part of this model is the Ubiquitous Language, which unambiguously expresses domain concepts, enabling effective communication. Any change in that understanding or expression needs to be reviewed by the rest of the team, to keep everyone on the same page, and to consider potential impact to the software design. Not every story changes the Domain Model.

Demo Script

How the dev team will demonstrate to stakeholders that the story is functionally complete. This would involve the dev team and Product Owner. Depending on team/org structure, more or less of this might be provided to the dev team by PO, or might even be developed collaboratively, in which case a separate review would be unnecessary. 

There is not a lot of detail here, just a handful of user-facing scenarios. If it is more than a handful, the story is probably too big.. It shouldn't take much time at all to produce or review. If it does take a long time, again it's probably too big, or there is a lack of understanding that needs to be addressed before proceeding. For example:

• Create an event
• Go to the Event list page
• Click "New Event" button
• Fill in information in Event form, click save
• Should show Event overview page with saved info
• Go back to Event list page, new event should be there
• Register for an Event
• Registrant Role
• Go to Event Registration page Fill in form, submit
• Admin Role
• Go to Event overview page, see new Registration

Design

For a microservice, this would focus on the API and its test cases, but would also cover important implementation details. Generally should be developed collaboratively with the major client(s) of the API, e.g. UI. In which case, the review with the rest of the team shouldn't take long.

• Resource endpoints
• Methods on those endpoints
• API objects
• API test cases
• API versioning
• New runtime dependencies, e.g. now we're going to have to use the Org service
• Major changes to infrastructure, e.g. using a new database, new message queue
• Data migration

Pair Programming

This is effectively continuous code review between two team members while the code is being written. I wrote up most of my thoughts on this practice here. Because of the normalizing and quality control forces exerted here, the pull request and code complete reviews go faster.

Pull Request

Code gets merged into the shared repository by issueing a pull request. This should occur frequently, typically multiple times per day per developer. It should be coherent and correct, but not necessarily complete. Each pull request gets reviewed by one or more other team members. In my experience, the types of things caught here are team standards violations, code/test readability (e.g. method and variable names), and code factoring.

Code Complete

When the dev(s) working on a story consider it code complete, the code and its tests should be reviewed by the rest of the dev team, typically plus an architect.

• The person(s) who did the coding walk everyone else through it
• Generally start with brief review of the story
• Then walk through the important parts of the code
• And the tests!
• While those persons are presenting, someone else is taking action item notes
• Somewhere public, or just email out after
• The story is not considered dev-complete until those action items are addressed.
• Creating a tech debt story to do it later is a valid option but should not be abused 
• This is at a level similar to Design review, focusing on API classes and tests, and any deviations from the original design.

How to Review

When to review?

Each work product should be reviewed as early as feasible, because course correction is cheaper the earlier you do it. Also, the less time elapses between creation and review, the more of the context the creator(s) will still have loaded. Finally, by spreading the reviews out over the lifetime of the story (as opposed to reviewing everything at the end), you avoid the tendency to get mentally overwhelmed by the volume of work to be reviewed and just rubber-stamp it. 

On my team, we had one-week sprints, Monday to Friday. We took Monday morning to commit to a set of stories for the sprint, produce and review demo plans for all the committed stories, and produce and review design and test plan for the first one or two stories we were starting work on. All other reviews were performed as needed at the end of each day.

Why not just create together instead of reviewing later?

There is a tradeoff to be made between direct collaboration and ex post facto review. In my experience, you typically want to keep creative activities for a given story limited to 2-3 people, 4 at the most. Otherwise the cost/benefit starts to deteriorate.

How much time to invest?

For a team that has been together a while, the amount of time to target for each review is ~30 minutes for code complete review, and 5-15 minutes for the others. Expect more review time early on in a team's formation. However, team norming will drive the time down significantly, and performing these reviews accelerates team norming. You don't keep finding the same things in review, because you incorporate the feedback and get better.

Who does the reviewing?

The composition of the reviewing group varies based on what is being reviewed, and larger review groups will tend to use more person-hours, because:

• There are more people spending the time
• More people talk more
• The members of large groups will be less familiar with what is being reviewed

Putting it all Together: A Sprint in the Life of a Story

• Monday morning - Dev team meets to commit to stories for sprint, and devise demo script for those stories. Right after that meeting, they meet with Product Owner to review the demo script, adjust as necessary.
• Monday afternoon - One pair of developers start on a medium-sized story. They come up with a design together. During the design, they realize that their Domain Model and Ubiquitous Language will need to change. Previously, Constituents could only register for Events. Now a Constituent can be designated as the "Event Organizer". They ask the rest of the team and an architect for a design review, scheduled for 30 minutes later. In the mean time, they check the proposed new terminology with the Product Owner, and add the term to the project's wiki. Then they go ahead and start implementing the design.
  In the design review, an overlooked test case is identified and added, and the format of a timestamp field is changed to suit the UI. Minimal rework in the codebase is required. They also take that opportunity to inform the rest of the dev team and the architect of the new Ubiquitous Language term. Monday end of day - The pair issues a pull request. One other developer is still in the office, spends 5 minutes reviewing it. Catches a few issues, which he comments on, but nothing serious, so he merges the pull request.
• Tuesday, Wednesday - Several more pull requests issued. Since the rest of the team is there, they both review both pull requests. The architect notices the pull requests going by and adds a few comments as well. Late Wednesday afternoon, the pair working on the story determines that it is code complete, and schedules a team code review for end of day.
• Wednesday end of day - Dev team + Architect code review for the story. One of the dev pair refreshes everyone's memory about the story, then walks through the code and tests. The other member of the dev pair is taking notes. A few minor issues are caught, and one potentially serious scalability issue. The team decides to consult historical metrics to see if it will be a problem short- or long-term. Once they determine that it would take about a year of usage at current rates to become a problem, they decide to add monitoring for this story, and add a tech debt story to address the scaling longer-term.
• Thursday morning - Dev pair demos the story to the Product Owner according to the demo plan.

Further Reading

• Code Reviews: Just Do It - Quotes some great case study numbers on the efficacy of code review
• Effective Code Reviews: 9 Tips from a Converted Skeptic
• Smart Bear: Best Practices for Code Reviews
• Atlassian: Creating Optimal Reviews
• Domain-Driven Design: Tackling Complexity in the Heart of Software, Ch. 2, Section 1: Ubiquitous Language
• The Art of Agile Development: Ubiquitous Language - Shorter version of above - definition and benefits of Ubiquitous Language
• Pair Programming Benefits - My experience with Pair Programming
• Pair Programming Benefits - Another blog on the pluses and minuses of Pair Programming

More Vagrant Learnings

Last time I wrote about how we use Vagrant to manage our local development environment at work. Since then I've dug into Vagrant a little more deeply and found a few nuggets I wanted to share.

Package your own Vagrant Box

There are some great base boxes (images) available for vagrant. It will even install them for you. If you want to tweak the box, you can use one or more of the many available provisioning providers. However, sometimes you might want to just bake the tweak into a new base image, for example, if the tweak is something you want to apply to all your instances, or if it is time consuming. Vagrant makes it very easy to do this.

Initialize, start VM

$ vagrant init hashicorp/precise32
$ vagrant up

Install new stuff in VM

$ vagrant ssh
vagrant@precise32:~$ echo 'set editing-mode vi' > ~/.inputrc
vagrant@precise32:~$ sudo apt-get update
vagrant@precise32:~$ sudo apt-get install vim
vagrant@precise32:~$ exit

Package up VM image as a new Vagrant box

$ vagrant package
==> default: Attempting graceful shutdown of VM...
==> default: Clearing any previously set forwarded ports...
==> default: Exporting VM...
==> default: Compressing package to: /Users/ryanmckay/projects/vagrant/package.box

$ ls -l
total 801784
-rw-r--r--  1 ryanmckay  staff       3001 Mar 14 22:13 Vagrantfile
-rw-r--r--  1 ryanmckay  staff  410509096 Mar 15 20:32 package.box

$ vagrant box list
hashicorp/precise32 (virtualbox, 1.0.0)

$ vagrant box add --name 'ryanmckay/withvim' package.box 
==> box: Adding box 'ryanmckay/withvim' (v0) for provider: 
    box: Downloading: file:///Users/ryanmckay/projects/vagrant/package.box
==> box: Successfully added box 'ryanmckay/withvim' (v0) for 'virtualbox'!

$ vagrant box list
hashicorp/precise32 (virtualbox, 1.0.0)
ryanmckay/withvim   (virtualbox, 0)

Provisioning

Vagrant provides a boatload of provisioning providers, e.g. puppet, chef, ansible, and even shell script. Normally configured provisioners are run when bringing up the VM for the first time. However, you can also run the provisioners with vagrant provision. For example, if you have the following:

provision_counter.sh shell script

echo "provisioning"
if [ -f /etc/provision_count ]; then
 counter=`cat /etc/provision_count`
else
 counter=0
fi
counter=$((counter+1))
echo -n $counter > /etc/provision_count
echo "provisioned: $counter"

Configured in your Vagrantfile

config.vm.provision "shell", path: "provision_counter.sh"

When you vagrant up, provisioners are run

$ vagrant up
...
==> default: Running provisioner: shell...
    default: Running: /var/folders/k5/6mc1_m_n1675572ts12qr7588mtjh5/T/vagrant-shell20150329-86419-qkjn7x.sh
==> default: stdin: is not a tty
==> default: provisioning
==> default: provisioned: 1

And you can run provisioners explicitly

$ vagrant provision
==> default: Running provisioner: shell...
    default: Running: /var/folders/k5/6mc1_m_n1675572ts12qr7588mtjh5/T/vagrant-shell20150316-31070-d35qp6.sh
==> default: stdin: is not a tty
==> default: provisioning
==> default: provisioned: 2

You can even add/modify provisioning lines in the Vagrantfile of a running VM, and when you run 'vagrant provision', the new lines will be used.

Reloading Vagrantfile

The Vagrantfile is the main config file for your vagrant VM. It specifies all the stuff that needs to be set up before the OS is running, as well as the hooks for provisioning once it has booted. The Vagrantfile can be reloaded by 'vagrant reload', which is basically 'halt' and then 'up'. It does not re-run provisioners unless you tell it to. Some examples of why you would want to do this are to modify your network setup or your shared folders. For example, if you want to:

Add a network port forward and a shared folder

config.vm.network "forwarded_port", guest: 80, host: 8080
config.vm.synced_folder "src/", "/srv/website"

Then 'vagrant reload' would restart your VM with the new configuration without having to reprovision.

Vagrant SSH

'vagrant ssh' is how you ssh into an interactive session on your VM. You can also use 'vagrant ssh -c command' to run a command in the VM instead of an interactive shell, similar to 'ssh host command'.

$ vagrant ssh -c 'hostname'
precise32

In order to connect to your VM with regular ssh (or anything that depends on ssh, e.g. scp or rsync over ssh), you need to

Export vagrant ssh-config

$ vagrant ssh-config
Host default
  HostName 127.0.0.1
  User vagrant
  Port 2222
  UserKnownHostsFile /dev/null
  StrictHostKeyChecking no
  PasswordAuthentication no
  IdentityFile /Users/ryan.mckay/.vagrant.d/insecure_private_key
  IdentitiesOnly yes
  LogLevel FATAL

$ vagrant ssh-config > ssh-config

Then provide ssh-config to ssh and friends

$ ssh -F ssh-config default 'hostname'
precise32

$ touch scp_me
$ scp -F ssh-config scp_me default:/tmp
scp_me                              100%    0     0.0KB/s   00:00    
$ ssh -F ssh-config default 'ls /tmp'
scp_me

$ mkdir rsync_test
$ touch rsync_test/rsync_me
$ rsync -a -e 'ssh -F ssh-config' rsync_test default:/tmp/
$ ssh -F ssh-config default 'find /tmp'
/tmp
/tmp/rsync_test
/tmp/rsync_test/rsync_me
/tmp/scp_me

Vagrant for Local Development

When I was hired at my current company about a year and a half ago, it was immediately obvious that this company took dev-ops much more seriously than any other place I had worked before. In fact, a lot of the dev-ops culture is pushed by the ops team, which is a dramatic, and very welcome, departure from my previous experience. This team follows a lot of the principles espoused in the Continuous Delivery book. In this post, I'm going to focus on the principle of making all pre-prod environments as production-like as possible, and in particular, what has been done in the local development environment.

The Ops team uses puppet for server provisioning in all environments. This facilitates the separation of the vast majority of configuration that is the same across environments and servers within an environment from the relatively small proportion that that needs to be different.

The Problem

Our project currently comprises 7 microservices and one CLI app, all based on Spring Boot. Most of the services are RESTful; a couple are message driven. They all expose a management port, primarily for doing health checks and gathering metrics. Deployable artifacts are built by Jenkins and stored in Nexus. Deployment is handled by a standardized shell script which is placed on the target servers by Puppet during provisioning.

We also use several 3rd-party applications, including MySQL, Mongo, Rabbit MQ, Splunk, and a CLI ETL application. These are also provisioned by Puppet.

The provisioning and deployment infrastructure is almost identical in production and the non-local pre-prod environments. However, until several months ago, provisioning and deploying in the local development environment (i.e. developers' laptops) was still a manual affair. This was achieved by following meticulously crafted documentation about how to set up a new workstation. Invariably, differences crept in, e.g. different versions of 3rd party apps, different configuration, even different package managers. Mostly these differences were benign, but sometimes they did cause problems. And that type of problem is much more difficult to diagnose and fix than a bug in the application layer.

Enter Vagrant

Vagrant enables you to "Create and configure lightweight, reproducible, and portable development environments." Vagrant VM images are called "boxes". You can produce your own box, or find one at https://atlas.hashicorp.com/boxes/search. Once you find one (e.g. hashicorp/precise32), getting it up and running is simple:

vagrant init hashicorp/precise32
vagrant up

Then you can ssh to it with:

vagrant ssh

And to stop it with various levels of severity:

vagrant suspend/halt/destroy

When you run init, vagrant creates a basic Vagrantfile in your current directory which has a little bit of configuration in it, and a lot of commented out stuff so you can see how to do common things.

How we use Vagrant

Our gradle build keeps updated a vagrant.config file which contains a simple ruby structure with information about our deployable services and cli apps:

PROJECTS = {
        'core-foo' => {
                :version           => '3.211',
                :project_root      => '/Users/ryan.mckay/projects/shared-services/core-foo',
                :use_puppet_config => true,
                :use_puppet_deploy => true,
                :im_just_a_jar => false,
        },
...

Our Vagrantfile

Load configuration about our deployables

load 'vagrant.config'

Configure some vm settings like memory and number of cpus

config.vm.provider :virtualbox do |vb|
    # Use VBoxManage to customize the VM. For example to change memory:
    vb.customize ["modifyvm", :id, "--memory", "5120"]
    vb.customize ["modifyvm", :id, "--cpus", "4"]
end 

Run external provisioning script

config.vm.provision :shell, :path => "../scripts/05-vagrant.sh"

Deployment (calls deployment script landed by puppet for each deployable)

 PROJECTS.each { |artifact_name, artifact_config|
    args = [
      artifact_name,
      artifact_config[:version],
      artifact_config[:im_just_a_jar] ? 'jar' : 'service'
    ]
    config.vm.provision :shell, :path => "../scripts/10-deploy.sh", :args => args
  }

Make sure everything came up

config.vm.provision :shell, :path => "../scripts/99-runtests.sh"

We use a private custom base image

  # The url from where the 'config.vm.box' box will be fetched if it
  # doesn't already exist on the user's system.
   config.vm.box_url = "http://foo.com/images/debian7-base.box"

Forward application ports to host system with 10,000 offset

   # Application ports
  (8000..9999).step(10).each do |port|
    config.vm.network :forwarded_port, :host => 10000 + port, :guest => port
    config.vm.network :forwarded_port, :host => 10001 + port, :guest => 1 + port
  end

3rd party service port forwarding

  # default intellij debugging port
  config.vm.network :forwarded_port, :host => 5005, :guest => 5005

  # mysql port
  config.vm.network :forwarded_port, :host => 3306, :guest => 3306

  # mongodb port
  config.vm.network :forwarded_port, :host => 27017, :guest => 27017

  #rabbitmq 
  config.vm.network :forwarded_port, :host => 5672, :guest => 5672
  config.vm.network :forwarded_port, :host => 15672, :guest => 15672

  #splunk mangagement port
  config.vm.network :forwarded_port, :host => 18089, :guest => 8089

Host/VM synced folder

  # create sync folder for integration test data
  local_sync_folder = "/tmp/foo_integration"
  FileUtils.mkdir_p local_sync_folder
  File.chmod(0775, local_sync_folder)
  config.vm.synced_folder local_sync_folder, "/mnt/filer/foo/foodev"

Deploy locally built artifacts if available

  PROJECTS.each { |service_name, service_config|
    if (service_config.key?(:project_root) && service_config[:use_puppet_config] != true)
        host_config_dir = service_config[:project_root] + "/src/config"
        if (File.directory?(host_config_dir))
          config.vm.synced_folder host_config_dir, "/var/bv/conf/#{service_name}/local"
          config.vm.provision :shell, :inline => "ln -sf /var/bv/conf/#{service_name}/local/dev-local.properties /var/bv/conf/#{service_name}/properties"
          config.vm.provision :shell, :inline => "ln -sf /var/bv/conf/#{service_name}/local/dev-migrate.properties /var/bv/conf/#{service_name}/migrate.properties"
        else
          puts "Project root found for #{service_name}, but src/config not detected. Local config directory was not mounted"
        end
    end
    if (service_config.key?(:project_root) && service_config[:use_puppet_deploy] != true)
      guest_folder = "/var/bv/apps/#{service_name}/local"
      local_artifact = localArtifactNameFor(service_name)
      host_lib_dir = service_config[:project_root] + "/build/libs"
      if (File.directory?(host_lib_dir))
        config.vm.synced_folder host_lib_dir, guest_folder
        command_to_run = "ln -sf #{guest_folder}/#{local_artifact} /var/bv/apps/#{service_name}/current.jar"
        if (!service_config[:im_just_a_jar])
           command_to_run += " && /etc/init.d/#{service_name} stop && /etc/init.d/#{service_name} start"
        end
        config.vm.provision :shell, :inline => command_to_run
      else
        puts "Project root found for #{service_name}, but build/libs not detected. Local build directory was not mounted."
      end
    end
  }

end

def localArtifactNameFor(service_name)
  version = PROJECTS[service_name][:version]
  major_version = version.split('.')[0]
  return "#{service_name}-#{major_version}.99999.jar"
end

Conclusion

Now that we have started using Vagrant, spinning up a new developer workstation takes a matter of minutes, and you know you got it exactly right. We can run integration and manual tests, and be confident in the result. And we are regularly exercising a good portion of the provisioning, deployment, and monitoring infrastructure that is used in production.