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 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

Client Side vs Server Side Session

Image
We recently looked at replacing our legacy session management system with a new one.  During this analysis, we came close to choosing a client side session but eventually concluded server side was better for us.  Here's why... Client side session In this model, all session state is stored in the client in a cookie.  The benefits of this are you don't need to worry about persisting and replicating state across nodes, session validation is lightning fast since you don't need to query any data store which means it's super scalable.  The session cookie must obviously be tamper proof (to prevent people creating a session of their choice) which is achieved by signing the cookie using asymmetric cryptography. The signing of a cookie value uses the private key, the validation uses the corresponding public key.  Our idea was to try and keep the private key as private as possible by storing it in memory only.  Each node (4 shown below) would create a new private and public
Read more