This website uses cookies. By using the website you agree with our use of cookies. Know more

Technology

Pull Requests Policy at FARFETCH Mobile

By Ricardo Santos
Ricardo Santos
Runner, music lover, electronics tinkerer, developing iOS apps since 2011. Adidas Ultraboost addict.
View All Posts
Pull Requests Policy at FARFETCH Mobile
Every team that has more than 5 or 6 developers may need to establish some guidelines in order to control the workflow of the source code that goes live.

Assuming you’re using git flow (even with forks), the more developers you work with, the longer the Pull Requests (PRs) take to get into the development branch. Certainly, no matter what kind of code-review techniques you adopt, your PR will be added to a queue to wait for its reviewers.

Along the way, sometimes PRs clash into each other, conflicts are generated, PRs are no longer eligible to be merged, interaction is required and you will end up in an almost endless queue.

I’ll explain how we’ve adopted 2 new dimensions to tackle this problem with the help of labels on PRs, cataloging them accordingly.

Buckle up, so we can show you how we’ve been dealing with this on the FARFETCH Mobile Domain.

Reviewer requirements

It’s important to have rules, otherwise, the whole process of getting source code live would be a mess, and we wouldn’t meet the minimum quality standards of code.
We have been using Impact as a dimension for cataloging our PRs for a while, and what’s better to decide if my PR needs 2 or 3 reviewers? Exactly. We will explain that in detail below. 

Impact

Like the name states, this is the impact of that PR in our codebase (is it a simple change? Or something that can potentially break the app?). This was something that we have been using for as long as we can remember. And it helped a lot to raise awareness of specific PRs that needed seniors and more people to take a look, before landing the development branch. The labels we use are: Low, Medium and High

Here are some examples:
  • Low - Changing a label
  • Medium - Creating a new UI Component
  • High - Changing a core piece, like the iOS AppDelegate or some core navigations of the app
With this in mind, it’s time to evaluate how many reviewers we need to merge a PR, not forgetting that we should nominate the person that is more familiar with that part of the code. 
  • Low - 2 or more approvers (can be from the same team)
  • Medium - 3 or more approvers (at least one from another team)
  • High - 4 or more approvers (at least 2 seniors, one from another team)

Prioritisation

We already covered the reviewer requirements but we figured out that the impact dimension was not the most appropriate for prioritizing the queue. It will not be a PR including a core component change that will have more priority than a hotfix PR.
  • How can we prioritise the PRs reviews in a fair way?
  • How can we make sure that these are being tackled and reviewed on a daily basis?
In order to answer these questions,  we have established guidelines to avoid PR collisions and, that way, making the process fairer for them.

A new dimension - urgency - is useful for prioritizing our queues. We’ll explain how this works in detail below. 

Urgency

This is related to the service-level agreement (SLA) time that the PR has to be merged. Of course, PRs pass into a code review stage, but some of them are more urgent than others. We use labels to categorise PRs and the labels we use inside the Urgency dimension are "Commitment” and "Urgent”. 

Here are a few examples:
  • Commitment - the PR contains code that the team committed to deliver on the current sprint;
  • Urgent - for PRs that contain changes that need to hit development as soon as possible. Things like fixing tests that are blocking the pipelines (even blocking a release) or a merge from master;
  • Regular - any PR that doesn’t have any of the previous labels is considered a regular one. No special rules apply.
With these labels in place, we can now specify the SLAs that we think are fair. Of course, we defined our SLAs in terms of working hours, because that’s what matters to us: how much time do our PRs spend on the queue, from when they are open until they are merged, based on a day of work.
  • Urgent - 4h
  • Commitment - 24h
  • Regular - 48h
But how did we come up with these values in the first place?

We agreed to start out with some values we thought were acceptable for the examples shown above: 4, 24 and 48 hours. We could then iterate on these later and properly refine the process along the way if the deadlines become too tight. Calling it a deadline may not sound right, or even fair. It’s more a guideline to the reviewers and to the owner of the PR that both of them will try to respect.

Validation of SLAs

As we needed some validation if these values were in accordance with our history of previous PRs, we developed an internal tool that analysed all of our PRs from the past and generated metrics for them on a weekly basis. Furthermore, it continues to analyse metrics each week, to keep the history up to date. Also, it generates daily warnings on a specific Slack channel, mentioning the people that have PRs that hit the SLAs.

It turns out that the SLAs we pre-established for our PRs were quite accurate. Since we only used High, Medium and Low labels in the past (before this whole initiative), we inferred the metrics by using those. 

These are percentile 85 measures from a universe of 2940 PRs (just from main app repository - yes, we also have repositories for internal frameworks):
  • High - 5.26h (289 PRs)
  • Medium - 18.81h (299 PRs)
  • Low - 11.40h (1782 PRs)
  • Uncategorized - 12.98h (570 PRs)
    • (these are the un-labelled ones, most likely fall into the Low category)
If we try to correlate High with Urgent (we know it’s not fair to assume that 100%, but it is an approximation) it’s close to the 4h SLA that we defined.

The same for Medium vs Commitment (18h vs 24h) and Low vs Regular (12h vs 48h).

Sounds good so far.

How did we build the tool that renders metrics and reports SLA infringement? What is its tech stack and what are the conclusions we have reached so far?

This will be the subject of another article, so make sure to subscribe to our F-Tech Blog so you don’t miss it! 

Flowchart

This is funny, but this whole initiative kind of started because of a flow chart. We had a lot of questions from our team members like:
  • "shall my PR be merged before the other one?”
  • "I cannot deal with more conflicts, I’ll be here the whole day!”
So we created a flow chart to help with these decisions, and based our prioritization on the impact dimension labels the PRs were required to have.

The flow chart contained guidelines on whether the merge requests should be merged before others or not. Let’s see an example.

  1. Is it urgent? No
  2. Is it commitment? No
  3. Is it the first in the queue? Yes
    1. Merge it
Another example:
  1. Is it urgent? Yes
  2. Is it the first urgent of the queue? No
    1. You should wait 
These are the kinds of questions that should be self-answered when looking into this flowchart, but more important than that: common-sense is applied here on every single fork.


Future notes

We’ve been iterating with this model since Q3 2020 and it has been proven to be very helpful in many situations, especially with the prioritisation.

In a future article, we’ll talk about how this is progressing and evaluate if we need to fine tune any step of the process to achieve the two important goals we have:
  • make sure we have a documented and respected process
  • reduce the PRs queues in the long term
Related Articles