Experimenting with GitHub Pull Requests
The Performance Zone is presented by AppDynamics. Scalability and better performance are constant concerns for the developer and operations manager. Try AppDynamics' fully-featured performance tool for Java, .NET, PHP, & Node.js.
Over the last month, the New Relic engineering team has been experimenting with GitHub Pull Requests. Until then, we’d been using GitHub for hosting our code, but not much else. For a long time, our workflow involved pushing features and bug fixes directly to our master branch. Whenever all of our tests passed, our CI server would recreate the ‘green’ branch from which we cut a release branch when we’re ready to deploy. Then, some time after pushing the code to master, another developer would take a look at the code and do a bit of QA to make sure any UI changes looked okay. We call this sidekicking.
What Prompted the Change?
As our engineering team was rapidly growing, our commit history was getting harder and harder to follow. More people were committing code more often, all directly to master. This meant our git was becoming very noisy.
We’re also deploying to rpm.newrelic.com more frequently — deploying almost everyday. As we work towards continuous deployment, it’s important to have a stable master branch. And since we’re deploying changes everyday, there is always pressure on the sidekick to get code reviewed after it hits master, but before it’s released to the public. Nobody likes a time crunch!
Besides, we were already using GitHub and loving it. Why shouldn’t we take it a step further?
Our GitHub Flow
Let’s take a look at how we manage our GitHub flow. First, we begin by branching off of master instead of pushing directly to it. We write our code and tests as usual, then push remotely to the feature branch and open up a pull request:
An @mention in the pull request body catches the attention of the sidekick and sends that person a notification. They can then come into the pull request at their leisure and provide feedback. Pull requests make the review process really nice. Anyone on the team can make comments on the pull request itself or dive directly into the diff and leave inline comments:
Once all is well with the pull request, the sidekick tells the developer:
We have developers merge in their own pull request so they have better visibility into when their code gets deployed.
Bonus: GitHub has recently introduced the ability to delete branches after merging in a pull request:
No more lingering remote branches!
With our new process, there isn’t much danger of a commit being cut into a release tag before it’s been sidekicked. Because of this, we have found ourselves with a more stable master branch – a very important step as we move towards continuous deployment.
There have also been a few other effects that we hadn’t originally anticipated. Sidekicks can now take their time reviewing the code before it makes its way into master, which relieves much of the stress. The readability of our commit history has also greatly improved as team members are able to fix up their commits and squash their feature branches.
And most importantly, the quality of our code has improved as people are giving feedback on code they weren’t responsible for sidekicking! Other developers are popping into pull requests (or even just individual commits) and providing feedback or suggestions. As a result, we’ve all been looking at a lot more code and offering improvement, and our own code has improved as a result of this exposure.
The Bad and the Ugly
Occasionally, we’ll find a pull request that stagnates for a variety of reasons. Sometimes a sidekick doesn’t realize they’ve been @mentioned. Or they may mean to get around to reviewing it but get caught up in other work. In others, they don’t realize that a particular pull request is more time sensitive than others. In almost all cases, communication goes a long way. If we need to a pull request merged in, we shouldn’t feel bad about doing a bit of prodding to make that happen. After all, it’s not unreasonable to remind a sidekick to take a few minutes and review the code.
There’s also been concern over long-running feature branches, which we define as any branch that’s been active for more than a couple of days. The longer a branch runs, the harder it is to keep it up-to-date with master. When this happens, our developers have to deal with merge conflicts and their work slows down because of it.
Interestingly enough, my own way of avoiding this problem has been to avoid keeping my branch up-to-date with master until I’ve actually opened up the pull request. More often than not, my feature branch is still cleanly mergeable into master once I open my pull request. If you’re working on a feature or bugfix that’s isolated enough, chances are you won’t have merge conflicts with others. Of course, this isn’t always a viable option, especially if you are handling multiple or large areas of your application. If you need to keep your branch up-to-date with master, it’s best to rebase against it at least once (preferably multiple times) a day.
We’ve also needed to make significant changes to our workflow around JIRA, our issue tracking tool, and we’re still working through some kinks with this process. Currently, we do a bit too much back-and-forth between JIRA and GitHub for our tastes, and we’re still working on automating changes we have to make on a JIRA ticket’s state.
Finally, it’s still a bit troublesome to QA changes to the UI before they’re merged into master. We’ve been investigating ways to easily spin up instances of our application using feature branches. This will let us double check changes to the UI before merging them into master and deploying them to a staging server. We secured three test servers, but each can only handle running one branch at a time. So we’re investigating divergence, a Ruby gem that maps subdomains to git branches so we can spin up arbitrary branches at will. This would essentially solve this problem, but we’ll need to allocate hardware to run it.
A Successful Experiment
All in all, we would call our experiment with pull requests a success. Despite the kinks I mentioned above, we’re all in agreement that our triumphs have been invaluable. We have a more stable master branch, more visibility into our code base, and even the code we’re writing is cleaner.
Have you or your team gone through a similar workflow change recently? We’d love to hear about your successes and how you’ve overcome the pitfalls!