Continuous Integration: Pull Requests, No Distractions

Many development teams use pull requests as a pillar of their development workflow. Pull requests are a formalized way of reviewing and merging a proposed set of changes to a codebase. Last month I hosted a webinar on Continuous Integration with Rob Bayliss of Last Call Media, and we discussed the ways his team uses pull requests in their projects.

A central theme in the recording is that pull requests—and all of the CI processes that get built up around them—should be optimized to improve the the communication of the team. Everything from the contents of the pull request, to the way it is tested, and ultimately merged and deployed should serve the team's understanding of the project.

What Is in the Pull Request?

For Drupal and WordPress teams, many pull requests will involve updating or adding dependencies—community modules, plugins, themes. Such a pull request might also add or change site-specific configuration files, custom templates, or CSS for the site. Team members reviewing the pull request probably care a lot more about reviewing the custom code than the added or changed dependencies.

But if your project's Git repository fully versions all of your dependencies, then your pull request adding and configuring a module contains thousands of lines of code that shouldn't be reviewed line by line. To mitigate this problem and make pull requests easier to review, Last Call Media uses Composer to manage dependencies, which recommends ignoring them in Git and only versioning composer.json and composer.lock files. Those allow all for exactly the same dependencies to be re-compiled.

Of course Pantheon's Git repository expects all PHP code necessary to run a site to be committed because we use Git as the deployment mechanism for your site. Rob illustrated the relationship between the the repo they use on GitHub where they make pull requests, and the repository on Pantheon used for deployment. Circle CI is used in between the two for testing and scripted deployment.

Image

Tabs vs. Spaces

Most of the teams I have worked on over the years have agreed to follow the coding standards of whatever system was being used for the project. In practice however, coding standards were sometimes only enforced when another team member saw a pull request with egregious violations.

The pedantic task of enforcing coding standards on a daily basis should not be imposed on a person. A system in which people have to manually point out each other's noncompliant indentation is one that generates unnecessary friction among those people. Your Continuous Integration system can run code sniffing and find any violations. Once team members know that the robots are handling that part of the review, they can focus on more thoughtful feedback.

Review on Green

With robots providing clear red or green marks next to each pull request, the people on the team have a clear signal of what is ready for review. I generally don't even look at PRs from other team members until I see green. Failing tests tell me that they aren't done working.

Even if tests are passing, they might still be working. I personally make a lot of "Work In Progress" pull requests. I indicate that these branches are not ready for a full review or merge simply by prefixing the title of the pull request with "WIP -". When the work is done, I remove the prefix. GitHub's labelling system is another great way to indicate status like "Help wanted" or "Needs Review".

How Do I Test This?

In the past, I have written some elaborate instructions for testing the pull requests I make. When I did so, it was usually the last step before marking the change as ready for review. Taking the time to write those detailed instructions made me feel like good team member who was making the task of reviewing easier for my co-workers.

I now think writing detailed, multi-step instructions to be read and executed by a person as my last task was an indication of an unhealthy process.

First, it was a problem that I writing these instructions last. By doing this step last I made the quality of those instructions a function of how much I wanted to be done with the ticket. If it was 4pm and I still had a lot of other work to do, then those instructions probably weren't very good.

Writing the testing instructions last also gave me ample opportunity to redo my work. In writing the testing instructions I would find flaws in logic and then fix my errors before asking a team member to review. Yes, finding your own bugs is better than relying on a team member to do it. But what if I had avoided the bugs altogether with clearer thinking thanks to writing the testing instructions up front?

Ultimately, writing testing instructions last is an indication that too little thought is going into the work in advance. Especially if the pull request you are making is the fixing of a pre-existing bug, the testing instructions should already be written. The bug should come with reproduction steps. Testing the fix should involve walking through the reproduction steps and not seeing the bug.

What Comes Next?

After merging pull requests, team members should have a common understanding of where the code goes next. If your team is doing Continuous Deployment, a merge should mean that the change is going straight out to live (possibly after passing additional tests). It is more likely that the change will be held until regularly scheduled release is made at the end of sprint or some other interval.