GitHub Code Reviews

Codeship NewsDevOps

Reading Time: 5 minutes

Update: We have released a free ebook about our workflow: Efficiency in Development Workflows.

Last week we showed you how we start with a new feature and the workflow to implement it. This week we will focus on the steps necessary to get your feature ready to be shipped.

Using Pull Requests

As I mentioned last week we use GitHub Flow on GitHub. But the whole workflow we describe is also possible when working with BitBucket. We do not have a policy when a pull request should be opened. Some of our developers open them when they start a feature, some wait until the feature is implemented. Then we push regularly to that branch as explained in the last post.

Open pull requests are helpful as everyone can see what we are working on. One really important part of feature branches and pull requests are proper commit messages.

The Codeship Workflow – Pull Requests

Writing good commit messages

As a distributed version control system, git makes it easy to take large changes and split them into small commits. Then these commits have a descriptive message with a short explanation in the first line and, if necessary, a longer description after a separation line. The git documentation project has some nice guidelines on it. Giving every commit a proper message and splitting large changes into a number of small commits makes your code history easier to read. It will be easier for your teammates to understand the history of your code.

I personally prefer git-gui for commiting and splitting up commits. It does only this job, but it does it very well. Take a look at some of the other git tools we like at the end of the post.

Using GitHub Issues and Labels

Once the feature is ready to be reviewed we assign its pull request to somebody else on the team. You have to read through the changes on GitHub at least once before handing the pull request . Making sure no obvious errors are in the code reduces the time and cycles necessary for a good code review. It is very helpful to inspect the changes outside of your own editor and with the excellent GitHub comparison view.

The Codeship Workflow – Comparison View

Now that we assigned the pull request, somebody on our team has a GitHub issue with his name attached. This way anybody can easily filter out the pull requests they need to look through. Now the last step is labeling the pull request with needs review. This way the reviewer knows this pull request is finally ready to be reviewed.

The Codeship Workflow – GitHub Issues

Code Reviews

When starting a pull request, the developer and reviewer can decide on looking through the pull request together or not. Especially for large changes reviewing the code as a pair is great.

Then we review the structure and syntax of the code, but we currently do not review the functionality itself. Before implementing a feature we discuss it in detail, so there is a common understanding. Once the feature is implemented, we trust our engineers that it does what it should. We usually don’t do feature reviews on our staging system.

One improvement we are starting with now is functional reviews for large and public facing features. Just so a second pair of eyes looks at changes that possibly impact a lot of our users. But this is still not a strict part of our workflow. A developer may request it after the implementation or we plan it before we start working on it.

When the review is done it either gets merged or, if the reviewer leaves comments, the GitHub issue is labeled as reviewed. This way developers can take a look at their open issues and see which have been reviewed and which need more attention.

The feature is ready to be merged as soon as the developer either updates the pull request or explains why she did it differently.

There’s one challenge with every release workflow that involves code reviews: Getting the team to review as fast as possible without interrupting their work. We try to do it asynchronously for smaller features and sitting down for shared code review on larger features. This is no silver bullet, but in our experience it works well.

Sign up for a free Codeship Account

GitHub Merge Button

To make sure nobody introduces changes into our production system without review, we have a strict rule that you can never merge your own code. This rule is not enforced through technology, but a strict team policy.

The only exceptions are scheduled releases, for example a maintenance Deployment. Then we let somebody review the changes and leave a ready to be merged comment. The build can then be merged whenever the developer wants.

By releasing through the GitHub merge button everyone on the team can push to production. Making this possible for every developer or designer in our team is very valuable. It allows everyone to take part in the process and it speeds up our daily work.

The Codeship Workflow – GitHub Merge Button

As soon as we hit the merge button our tests are kicked off on the Codeship again and the deployment starts. Next time we will go into more detail about this part of our infrastructure, how we deploy to our staging app, test our migrations, build our deployment pipelines and then push to production several times a day.

Your Development Workflow

How are you working? We would love to hear about it in the comments. Let us know if you have any questions or suggestions to our workflow. Your feedback is always appreciated!

Ship long and prosper. And have a look at part 3 here.

PS: Should you be interested in a convenient CI and CD solution, try Codeship.

Further info

Subscribe via Email

Over 60,000 people from companies like Netflix, Apple, Spotify and O'Reilly are reading our articles.
Subscribe to receive a weekly newsletter with articles around Continuous Integration, Docker, and software development best practices.

We promise that we won't spam you. You can unsubscribe any time.

Join the Discussion

Leave us some comments on what you think about this topic or if you like to add something.

  • Gregor Dorfbauer

    Nice writeup, Flo.

    I agree mostly with your points, but not having a staging system in place is a no-go for me. I like to have an exact copy of the production system to do a what I call “real integration test”, as abstracting your system for testing (stubs, fixtures, …) could introduce bugs which only occur in the real world.

    I’ve made a similar list a few weeks ago about my learnings in web development in the last year:

    • I agree that having a staging system is helpful, but mostly for review purposes. The problem I have with a dedicated staging process is that it slows down the development dramatically.

      For example we push new stuff all the time and from time to time we get it wrong. But we know that whenever a bug happens we can fix it quickly as our changes are small and we can push it out quickly. The faster the deployment can happen the less important a dedicated staging environment becomes.

      Most systems, especially typical webstacks, can be reasonably well built on a local developer machine with tools like Vagrant. This is close enough for development to deliver quality software and with proper tests we really don’t miss a dedicated staging environment.

      Systems that have longer deployment times a staging environment can be helpful, but bringing down the deployment time is still the better way to go there.

      Having a development environment that is close to production is important of course to not rely on staging.

  • americanbreakfast

    Do you have any advice for developers working alone where a peer review system doesn’t exist?

    • I’d still go with the branching and pull request model and then at least do a code review yourself before merging. This provides a much better quality control than when being alone. Although of course this takes more time and feels a bit slow during development. Put it pays off

  • I’ve watched Zach Holman’s video on the topic and read through your posts and one thing is consistently missing: QA Testing. When do you actually test and use the features that were committed? Do the business folks see the new features for the first time in production??

    • We don’t have any manual QA process in place. We do ad-hoc feature reviews if necessary, discuss the features before or ship a simple version that maybe only the developer has fully seen with the expectation to ship a nicer version soon after.

      We really trust the good judgement of our developers to implement something good and extendable.

      We are currently a small team, so communication around features is still easy. We will have to introduce new steps there in the future. The first will be to discuss larger new features now more during the design phase.

      But we truly believe in shipping quickly and then building on top of that.

  • billycravens

    How should I manage pull requests and Codeship? When I push to that branch, it does a full CI run – then when I merge into master, it does another. For a small team, that means master is identical to the most recent feature branch, so this is redundant. Not only does this take time, but it burns up my monthly CI run limits.

    I know I can limit Codeship to just the master branch, but that defeats the value of CI for me – it’s nice to look in the pull request and know if it’s a good build.

    Thoughts on this workflow?

    • >For a small team, that means master is identical to the most recent feature branch, so this is redundant

      What we’ve seen is that when pushing and merging to master regularly the two branches aren’t really the same. We will be working on better notifications and dashboard, so the different branches are easier visible.

      We are thinking about changes to our pricing as we do not really want people to punish for pushing regularly. Of course all grandfathered for our current customers.

  • Pingback: Using Pull Requests for commercial/private/proprietary development | Robert Daniel Moore's Blog()

  • To make the actual review process easier and to keep track of which comments of the reviewer have been addressed by the developer we use

    It keeps track of unaddressed comments and discussions in your pull requests.

    • Looks great, but it’s another service that needs full access to the repos, which is a no-go.

  • Pingback: Ceiba3D Studio | Repository Driven Infrastructure | via @codeship()

  • Mariano

    Great post! I definitely agree mostly with the items you mentioned. I’m Mariano Focaraccio, founder & CEO of Gitcolony ( and we are working really hard to redefine how code reviews and QA are managed.

    Basically we think that pull requests are great for open source contributions, but definitely not the most efficient way to reinforce an internal code review process in a company.

    We consider that if you work a week (or even 2 weeks) on a feature (since you use a branch-per-feature schema), that PR would be a titanic review. One of our mantras is: “Ask a developer to review 10 lines of code, he will find 10 issues. Ask him to review 500 lines of code, he will say it looks good”. Based on that, we introduced the partial review concept with our “Live branches”, so you can review code as it’s been built (using a similar schema to PRs).

    In addition, we included a business rules engine with dev votes and QA votes, CI plugin, integration with tracker, etc so you can define different policies (build broken, open issues associated to a branch, a minimum number of votes, etc) to define if the PR/branch is ready to be merged.

    I would love to hear your thoughts about this!