Professional teams that produce quality software and maintainable systems often employ a lightweight process to safeguard the introduction of new or updated code to their stable branches. A code approval policy is an agreement within a single team or an entire organization to follow a set of rules regarding how and when code is accepted into the main lines of a project, on the way to reach "production" and final distribution. In short the way a team approves code for a new feature or bug fix making sure only what is good and high quality gets merged.
For teams that use Git, a common way to adopt such a policy is to enforce merge checks at code review or pull request time. Check our documentation for the definition of pull request. There's a lot of freedom in how teams can define the conditions required before a code change can be merged. And the options are not mutually exclusive, on the contrary, it might help to mix and match. Let's review a non-exhaustive list.
Require at least X reviewers to approve the change
The easiest policy is to enforce that a few people look at the new feature or bug fix before it's merged. For example, many teams decide that a pull request can only be merged if at least two developers have reviewed and approved the code. Your team may want to set an upper limit on the number of reviewers to prevent slowing down the progress too much but it's often useful to invite more reviewers than the minimum approval limit so that the progress on the review is not stalled by busy team members.
Require that feedback on code changes has been processed before merge
This might come without saying, but after a reviewer has provided feedback and suggested amends, code should not be merged until those suggestions are either implemented as requested or at least discussed and a consensus is reached. Failing to do so defeats the purpose of having the review in place. Bitbucket Server offers tooling to this effect.
Require a set number of successful builds before merge is possible
Another approval policy is to enforce that a certain number of builds are green - all the tests pass - before the code can be merged.
The requirement for multiple successful builds can stem from the fact that a repository contains different components which have to be built separately, or from the fact that a "build" can sometimes be interpreted as a "successful deployment to environment x". In other cases it's a mechanism to protect the team from flaky tests.
Over time some projects grow flaky tests - i.e. tests that fail randomly once in a while and are surprisingly hard to troubleshoot or refactor. Because of this some teams require X number of successful builds before merging a piece of completed work.
Inside Atlassian, the Bitbucket Server enforces all three checks mentioned above. For a developer to be able to merge a pull request, he needs at least 2 approvals - from any member of the team - and the build needs to be green.
Require performance tests to improve on a set baseline
Teams that have services sensitive to performance degradation can put in place policies related to that as well. For example, a policy might dictate that piece of code cannot be merged onto a stable release unless the overall performance target benchmarks are met.
Require architectural or systems impact review
Teams might also put in place policies that are related to seniority review and systems thinking. They can nominate a gatekeeper, an Architect to oversee major changes to the code to make sure that systems stay maintainable, scalable and performing. In addition to that, domain experts might be involved with reviewing code that is within their expertise. If a developer is making a change to a load balancing algorithm on the back end, he may want to check his work with the original author of that code and with a Software Reliability Engineer to be sure he is taking all potential problems into account.
Per branch policies
Approval policies can differ per branch: it might be enough to have two reviewers approve a pull request to move the code to a development environment but a team may also want green performance tests before merging the feature onto the pre-production system.
Code Approval: Speed vs. Process
In deciding and enforcing a code approval policy this is always something to remember. The more laborious the process, the more roadblocks the team puts in place to ensure proper review, the slower the development progress will be, at least in the short term. In the long term, one would argue that the high effort on high standards will result in less maintenance costs for the team.
At Atlassian, there is consensus in all development teams that code approvals are important and should be enforced for all. Teams are empowered and free to adopt the model that they think suits them best.
You will recognise some similarities between the policies listed above and how we shaped our products to support an effective code approval policy. Bitbucket Server allows teams to setup pull request checks with a few useful conditions:
- Require a minimum number of successful builds
- Require a minimum number of approvers
- Require all tasks reported in the code review to be resolved
Because Bitbucket Server is also extendable via add-ons it's relatively easy for a team to create custom flows that resemble the policies defined inside their own organisation.
How does your team do code approval? Let us know at @atlassiandev or in the comments below!