Pull Requests encourage critique, even in small teams

A member of our team recently suggested that we use pull requests in our development workflow.  Many points were made for and against in our debate.  After only a short time of adopting this change, the major benefit I have seen is the increased critique of the team's output.

The debate

The decision to use pull requests was certainly not a given and gave rise to an interesting debate.  Many arguments were made for and against which I've tried to summarise below.

For pull requests

  • Increased knowledge sharing - Non reviewers can view pull requests just as easily.
  • Simpler commit messages - Each pull request is (hopefully) created "with the reviewer in mind" .
  • Provides a forum for commenting on work.
  • Increase in code quality


Against pull requests


"It's too much process and will slow things down"

As a small team I think we take pride in our lightweight process of pair programming and informal ad-hoc code reviews.  Adding extra checks and process to the deployment pipeline feels almost like a step backwards as we try to cut back on red-tape.  

Counter argument: No other rule would be introduced alongside using pull requests.  This means anyone in the team (except the author of the change) can accept anyone else's request.  Where possible, we'd aim for one pull request per user story.  Perhaps this argument/concern was a fear of pull requests just being the start of a new heavy weight approval process?


"Pull requests lead to bigger and later code merges than the smaller earlier merges of committing straight to master"  

Given that a pull request should encapsulate all changes for a single user-story (or task), this is somewhat inevitable. 

Counter argument: This issue is mitigated by staying focused on the task/story in hand.  If the developer(s) happened to get sidetracked and begin re-factoring, this should be split into a separate pull request.  If a pull request does encapsulate a single user story, task or refactor it becomes easier to review and understand changes made to the code base.


"We pair program... we don't need a extra reviewer"

Counter argument: A fresh pair of eyes can spot things that even a diligent pair might miss.


Benefits I have seen since adopting pull requests


They encourage critique in a way I didn't expect

Pull requests seem to frame a change as a question and not a statement.  My impression is that:

  • Committing to master states: "I am done with this".
  • Submitting a pull request asks: "What do you think?"

It is so much easier to make a comment or suggestion when someone asks for your opinion than when you initiate the dialogue yourself.  The more that comments and suggestion are encouraged, the better the team's output will get.





Comments

Post a Comment

Popular posts from this blog

Lessons learned from a connection leak in production

We recently encountered a connection leak with one of our services in production.  Here's what happened, what we did and the lessons learned... Tests and Monitoring detected the issue (silently) Our automated tests (which run in production for this service) started failing soon after the incident occurred.  We didn't immediately realise since they just went red on the dashboard and we just carried on about our business.  Our monitoring detected the issue immediately and also went red but crucially didn't email us due to an environment specific config issue. The alarm is raised A tester on the team realises that the tests had been failing for some time (some time being 4 hours... eek!) and gets us to start investigating. Myself and another dev quickly looked into what was going on with our poorly  dropwizard  app.  We could see from the monitoring dashboard that the application's health check page was in error.  Viewing the page showed us that a downstream ser
Read more

How to test for connection leaks

Connection leaks are easy to introduce, hard to spot, can go undetected for a long time and have disastrous consequences. It's not enough to fix a connection leak or code against them, you must write tests that would fail in their presence.  Here I will discuss a number of techniques that could apply to many different languages and frameworks.  I have also provided some concrete examples on github which make use of Dropwizard , JerseyClient and WireMock . What is a connection leak? Each time system A talks to system B over a protocol like http, it requires a TCP/IP connection to send and receive data.  Connections can be re-used for different requests, via a connection pool, or a new connection can be established for each request.  Once system A has finished with the connection, it needs to either return the connection to the pool, or close it.  If this doesn't happen you have a connection leak.   Given a connection leak, everything will continue to work ju
Read more

How to connect your docker container to a service on the parent host

Recently I needed a docker container to access a service running on the parent host.  A lot of the examples on the internet were very close to what I needed, but not quite. First off, docker's documentation provided an example that (as far as I can see) doesn't actually work.  The following (I think) is meant to inject the src ip address of the parent host's default interface: $ alias hostip="ip route show 0.0.0.0/0 | grep -Eo 'via \S+' | awk '{ print \$2 }'" $ docker run  --add-host=docker:$(hostip) --rm -it debian What it actually does is inject the ip of the default gateway on the parent host which doesn't help us. Michael Hamrah 's blog details the " gateway approach " of getting the parent's host ip from within the docker container.  This works great, although I wanted to inject the ip on container run and not have that logic within my docker image. Inject the src ip of the docker interface I think the best
Read more