Pull requests are fantastic

As you may know, my favorite way of beginning a blog post is with “as you may know, my favorite way of beginning a blog post is with ’as you may know’”. As you may also know, for the past year or so I’ve been working with Silent Circle, where I’m part the team that develops the website and backend.

The team is composed of roughly seven people now (developers and designers), and, as it has grown, we have been trying things to find a good workflow that allows us to be productive while staying on top of the changes made to the code by coworkers.

This post is about our needs and the development/release workflow we have arrived at to address them.

Creating a workflow that suits you

First of all, I should say that not every workflow will match every team. The workflow we use won’t suit most teams, but it will probably suit many, which is why I’m writing this post. Depending on the way your team does things, you may find that there are some elements in our workflow that you like, or you may find that you do things completely differently.

Both are perfectly fine, the way a team works should match what they feel comfortable doing and the nature of the product itself. For example, since our product is high-risk, we can’t afford a very rapid release cycle (multiple times a day), so we push to production once every week or so. Your mileage will vary.

Enumerating your needs

When starting to create your workflow, you should think about and enumerate your needs. These will be roughly what every other team has (most people have to write code and make sure it somehow gets to people), but there will be intricacies that are specific to you. Those are what you should focus on to come up with a system that works well for you.

In our case, some of our needs were:

  • We have senior developers and junior developers, and we would like the senior developers to be aware of the changes the junior developers are making, to make sure they are correct, that they follow the spirit of the codebase, the coding style, that they are necessary, etc.
  • We would like developers (especially the junior ones) to be able to make changes on their own, without worrying about stepping on other developers’ toes.
  • We wanted people to be able to ask for and receive feedback on what they were working on, even if it were unfinished and not yet in a state to push to production or to integrate into other developers’ work.
  • We would like the code in the repo to at least not be broken (i.e. we’d like it to pass static code checks) and adhere to the codebase’s style.

Our workflow kept evolving from a team of two people up to now, so we went through various stages of things that “worked for us” to arrive to the current “thing that works for us”.

Our current workflow

The workflow we’re currently using is mainly based on git-flow, with some ancillary changes that were dictated by the tools we use (mainly Atlassian Stash). As I mentined earlier, our product needs to be thoroughly vetted before going to production, so we have a QA team testing release branches before finally pushing to production. We would like to use something like GitHub Flow, but it’s not a good fit yet.

What will typically happen for a bug or feature request is:

  1. The reporter (a developer, user, QA person, or support person) will open a ticket detailing the problem or feature request. This will be assigned to the developer who is more fit to deal with it, usually by other developers. We know who wrote what and who has more experience with what, so, when a ticket comes in, we usually know who would be able to fix it the fastest. Sometimes people will also assign tickets to themselves if they think they can get them resolved quickly.
  2. The developer will branch off develop into a feature branch and will start working. As soon as some amount of work has been done on the issue, the developer will publish the branch and open a pull request with no reviewers. This means “I’m working on this”, and allows the rest of the team to have a look and see if the work there impacts their code, if they have any feedback or if they need to do something to interoperate with it.
  3. When the feature is almost finished, the developer will write tests to cover it, and will make sure all tests pass. We also have a dedicated Jenkins server running tests on every commit, so we are notified right away if something breaks.
  4. When tests have been written and passing, the developer will add a reviewer (usually one of the senior developers, or any other developer the former would like to look at the code). This means “this branch is ready to merge”. If it’s a merge to master, the head of QA will also be added as a reviewer and be required to give her go-ahead before the branch is merged.
  5. The reviewer(s) will look at the code, give feedback if there’s something that needs to be changed or accept the pull request and merge it into the specified branch. If QA is required, the QA team will make a pass through all the issues touched on by the pull request, test them one by one and give the green light if everything is okay. Afterwards, the request will usually make it to the server within a few minutes, since we aim to always be able to run the head of any given branch on the servers it has been deployed on. This eliminates questions of the type “should this commit in master go to production yet?”. If it’s on master, it should be in production.

Pull request reviews

Since we want developers to have visibility into code changes, we require some number of developers to have approved a pull request before it can be merged. The value of this depends on the number of people working in the team and what you want to accomplish, so it may be anywhere from “0” to “everyone”. We only require one developer, but usually a minimum of two or three at least take a look at the proposed changes.

This lets everyone be aware of what everyone else is working on in real-time, and also gives developers the confidence that what they are working on will work properly with parts of the code they don’t know as well, by allowing people to comment (or by requesting comments). It has worked very well for us, and I would personally recommend it.

Automated checks

A robot, running scaredhttps://farm3.staticflickr.com/2609/3888108984_4f8239c884_n.jpg">Automated code testing is good for everyone.
Almost.

As I have written before, we also perform automated code checks. Apart from running our tests in Jenkins (and locally), we also run static code checks (which are much faster) on every commit, and have configured the server to not accept broken commits by using git’s pre-receive hooks. Pre-receive hooks take place on the server and aren’t perfect, though, as they usually require commits that fix the previous errors, and are a bit spammy in the commit log.

For that reason, we have also implemented local pre-commit hooks on each of our development machines (and usually our editors have some plugin to do the same, I use syntastic for vim). This lets us catch typos and other errors like that before committing, as git won’t accept the commit if there are any syntax errors at all in the code.

This is a very powerful way of catching errors, and it’s so easy to set up and maintain that I would suggest that everyone have some variant of this. It only adds around a second to commit times (but it depends on your codebase), so it’s a very quick way to ensure that no errors have slipped by you.

Epilogue

This post was a short introduction to a development workflow that I have personally had a good experience with. I would love to hear about the workflows you use and would recommend (or would not recommend, for whatever reason). If you have anything you want to say about how you or your team work, please leave a comment below, send me an email or tweet at me. Thanks!