Pull Requests – 2 Months In

The InterfaceBlox team has recently adopted a few development processes such as short sprints and using pull requests as code reviews. This is the first post in a series about lessons learned and few new ideas with which to experiment.

How We Work

We adhere to the branch per issue strategy, where each JIRA issue gets it’s own branch. We name the branch after the ticket number so it is easy to reference in the future.

When an issue is complete, we submit a pull request for that branch. The pull requests offer everyone a chance to both review the code for bugs but also serve as a way to share knowledge of different parts of the system. The knowledge sharing aspect is actually quite important for our team as half of us are remote and 75% of us are new to the team and the codebase. Overall this has worked quite well in both regards, but there is still room for improvement.

Lessons Learned

Review Early & Often

For a typical procrastinator like me, it’s almost natural to wait until the last day of the sprint to start reviewing other people’s pull requests and judging by our team’s behaviour, I’m not the only one. The problem we’ve run into is that if you don’t review the code until the last minute, you often don’t have time to make the suggested changes before the end of the sprint. Additionally, if many pull requests start getting merged all on one day, there is a high probability these parallel branches may conflict with one another, trigger build & test failures. The solution, of course, is to review the code as soon as possible.

Check Out The Code

Many pull requests are easy to understand just looking at the diff online, but for more in depth refactorings, it’s often a good idea to load the branch into your IDE in order to poke around. You’ll find more issues this way as well as you’ll also see more of the existing unchanged code that may cause conflicts that you wouldn’t otherwise see in just a diff.

Improvements

I recently read a series of articles on how Github works. They do a couple additional things that I think could really improve our process.

Building Pull Requests

Very often we’ll approve & merge a pull request only to have it cause test failures in the build farm. These failures subsequently causes failures on many downstream builds that rely on ServiceBlox. Since the test suite takes approximately 1.5 hours we cannot expect developers to always run the full test suite. A solution to this problem, and part of Github’s dev process, is to trigger builds in the build farm for pull requests. In this way we can be confident our build is functional before we merge into the main branches and we can therefore ensure no other downstream builds will break.

Pull Requests as Documentation

Another interesting way in which Github works is that they often use pull requests as a way of exploring a new feature or refactoring. Typically we have tried to write a more formal specification document in Google Docs, with mixed results.

We’ve recently tried the pull request as documentation ourselves with Rafael’s “abstract predicate framework” but there was also a Google Doc. It would be interesting to try this a few more times without the Google Doc, trying to capture as much of the conversation and design in the pull request as possible. There are some nice benefits of using pull requests instead of formal specification documents:

  1. Documents tend to disappear into Google Drive, never to be seen again.

  2. Invariably once you start to code a feature, various assumptions you made in your document are incorrect. It’s hard to keep a document in sync with your code than it is to keep iterating on one pull request which can capture all the discussions over time and in context of the code at that time.

  3. It’s much harder to write a good specification document that reviewers can comprehend well enough to offer insight than it is to have a well documented pull request with code samples.

Please leave a comment.

4 Comments
  1. Zef Hemel 7 years ago

    Nice article! I think it’s a good idea to put some design documentation in the pull request, but of course it’s also good practice to do a little bit of design upfront and to discuss that before implementing and issuing a pull request.

    Also, for the record: we use bitbucket (with mercurial) at LogicBlox, which offers nice pull requests, but Github’s pull request system is very nice too.

  2. Thiago T. Bartolomei 7 years ago

    I think this post nicely summarizes our lessons learned. Another one that I found important is to let the developer who issued the pull request perform the merge in the branches for higher versions.

    For example, we have distinct branches for versions 3.9, 3.10 and 4.0. When a bugfix is implemented for 3.9, we often want to merge it into 3.10 and 4.0. The lesson here is that we should merge immediately after the request is approved for 3.9. This has 2 major benefits:

    1) we incrementally merge the changes up, we never leave it to a big bang merge that potentially causes many conflicts.

    2) the developer who knows more about the changes deals with conflicts (instead of the developer performing the big bang merge).

  3. Author
    Trevor Paddock 7 years ago

    FYI, the “How Github Works” articles are actually about how the company works not how Github the product works. I actually read them to find out about how they manage being a mostly remote team.

  4. Jeff Vaughan 7 years ago

    I love the idea of having pull requests trigger buildfarm builds and tests. I wonder how hard this would be on hydra…

Leave a reply

© Copyright 2020. Infor. All rights reserved.

Log in with your credentials

Forgot your details?