Preamble
After seeing some recent tweets and blog posts by Martin Fowler and Kief Morris on the topics of Continuous Integration, Pull request reviews and branching and how we may not be doing it quite right at least internally within an organization(references below), led me to explore this practice a little further.
First, let’s define what Continuous Integration is
Continuous Integration is the practice of merging all developers working copies to a shared mainline daily if not multiple times a day.
I really like this idea as it brings with it very clear benefits including
– reducing the possibility of large merge conflicts
– reduces peer review effort as the code to review is smaller and thus easier to reason about
– smaller commits make it easier to find issues and roll back if necessary
A little background
I have been a proponent of feature branching and pull request review ahead of merging(pre-integration review) within an organization’s team for some time. The reasons are as follows:
Feature branching allows the reviewers to review a PR that encompasses the entire body of work that is aligned to a feature enabling them to know the scope of work associated.
Pre-integration reviews provide value in decreasing potential issues that otherwise would not have been caught by say unit, integration, functional tests and analysis, this is especially true if the codebase is a brownfield project that may not already contain a high level of test coverage.
The Issues
However, we noticed quite early on that there were some drawbacks to these practices such as:
Feature branches required an entire feature to be completed before a PR be created, well if a feature required several days to complete this right away violated the CI principle and thus does not bring with it the benefits above.
Pre-integration reviews incurred delays in completing tasks due to relying on team members being available to perform this review at the time of task completion, as team members were typically busy completing their own tasks we typically waited for them to complete their tasks to avoid them context switching unless there was a dependency on this task. So this meant the developer who completed the task would pick up a new feature to switch to and wait for the reviewer to be free, which may be when they develop in this case is now busy so no matter which way you went there was a hit on throughput/productivity and led to tasks taking many days to complete again violating CI.
As you can see these practices may not be the most effective way of going about doing things especially as speed is of priority which almost always is the case.
One exception to this where such a level of rigour may be required for compliance reasons.
So with feature branches and pre-integration review causing such issues I was curious about solutions to these problems or if these were simply a trade-off to accept for apparent higher quality. After reading further into the aforementioned blog posts there were a few good thoughts that made me rethink my ideas around quality oversight.
Possible solutions
Well, I think we have to look at how we may have got to perform such practices -> Open Source.
Pre-integration review and feature branches make complete sense for open source development as you are working with external contributors who may not have the same standards, styles and objectives as you, wherein an organization it would be assumed this would not be the case.
So what are the alternatives to these practices in an organization setting?
Instead of performing pre-integration reviews, we could apply pair programming or periodic code reviews:
Pair programming, one of the practices in XP, suggesting 2 programmers work on a task together one acting as a developer and another acting as a reviewer in real-time reducing the time of developing something incorrectly and re-implementing. Shortening the development – review – improve cycle
Periodic reviews where you can perform scheduled reviews of committed code, this takes an assumption that code committed is of a reasonable quality to be committed through training etc. and if there are issues they can be fixed through subsequent refactoring.
Instead of creating feature branches we could allow partially completed features to the codebase. This way you can push your changes as frequently as you like and be able to adhere to the CI principle or merging at least once per day. A side benefit of performing this rapid commit style is that it encourages refactoring, and following a clean as you go mindset. What I mean is that you can make any refactoring you notice during your development whether it’s part of the feature you are working on or not.
You might say well how do I do this as it will not be a fully working feature and we can’t release partial features to end users, well one popular approach to get around this exact issue is to begin using feature flags and switch on this feature & make it visible when ready.
Performing refactoring like this is a safer approach for two reasons, one is that it provides you with a better chance of getting to fixing an issue sooner rather than later before complexity increases or it becomes a more serious issue down the line and two hoping to get time at a later time to apply refactoring is a big challenge get buy-in as delivering features pressures will always be present and this is a good thing as we don't really want things to get quiet do we!
Summary
Again the above is not to say one way is necessarily better than the other, depending on your circumstances one way may be more appropriate than the other. This post is just to provide further options if you find yourself in a similar situation where such processes may be hampering your delivery velocity.
References:
https://infrastructure-as-code.com/book/2021/01/02/pull-requests.html
https://martinfowler.com/bliki/RefinementCodeReview.html
Top comments (2)
Fowler says:
I think this is an area, while technically correct, is something to seek the majority of the time. However I don't see a benefit to push unfinished work to a shared mainline.
Importance of Pull Requests for Skilled Programmer
Jesse Phillips ・ Jan 6 ・ 4 min read
When I have created a merge request, I find many times after I use it for a bit that I did not get something. And I know that even before I get to a merge request I will work through a number of important items. I can't imagine trying to meet a once a day integration over getting clarity on the work I'm doing.
As QA I do have the benefit that I'm also the user of the code I write. ButI'm still building infrastructure, changing architecture, and trying to break apart the pieces I believe to be complete from those that still needs work.
Hi Jesse,
Thanks for your comment. I fully understand the concerns on this and I had the same concerns due to potentially causing more pain & headache due to the extra effort in co-ordinating finished vs unfinished work.
However, my thoughts around this are not necessarily a right vs wrong scenario rather an ideal to work toward and put in place the necessary processes to allow for unfinished work to be committed such as feature flags etc.
This will allow you to commit not only unfinished work but to your point work you may soon later realize there is a different perhaps preferable approach and update this code ahead of going live.
Yes, there will be issues committing unfinished work if safeguards are not in place however I don’t think this is a reason not to do this as there are definitely advantages of committing with this frequency. So, I think the path is if this is not possible right now - fine however working towards this over time will ultimately yield superior agility.
I would almost compare this to DevOps - there are upfront costs associated with putting in such processes and infrastructure but you believe will pay off afterwards.