Abandoning the "no file overlap" stage merging rule

[mokimo]

Context

We initially did not want to allow the automated merging process to merge multiple PRs touching the same file, to make it easier to revert PRs. However as the process is now pretty stable and the QA process also catches a lot of critical issues before they go onto production, we should consider dropping this rule.

Additional context

If Git/GIthub actually can't merge anything due to overly complex file overlaps, this will be prevented and highlighted in the UI and an engineer needs to properly rebase their PR. That's something we can't and shouldn't solve, but we often have the case of changing a single line in utils.js which leads to a whole PR to being blocked for a longer while.

I recently had a PR that was blocked for 2 weeks while it was ready to merge from a testing/QA/git perspective. I've seen this happen and we currently have 'congestion' when someone changes a popular file such as the utils.

In case of issues

The only disadvantage that I see, is that in case of issues we might not only need to revert the offending PR but 2-3 if we run into any conflicts of just reverting the single PR. Given this would be a big corner case, and usually since we only merge "simple" file overlaps, we should be good in that regard.

Concerns?

If anyone has any concerns, I'd love to account for them - otherwise I'll start looking into getting rid of this self imposed limitation within our merge-to-stage process.

[narcis-radu]

We can give this a try and see how things work. I see these use-cases are not that frequent, and when we do stumble upon them, there aren't any code conflicts or edge cases that could create problems. We should also broadcast this internally and make sure Engineers are aware of the upcoming changes.

[honstar]

Thanks @mokimo for the write-up and suggested change 👍 I support this change, given your past diligent handling and great automation efforts, I'm confident that this will further contribute to a stable and efficient release process.

[overmyheadandbody]

I agree with the general idea, as it would further streamline the automatic merge pipeline. And although we don't have to revert things from prod all that often, I do want to consider that use-case.

PR1 goes first into the stage batch, then PR2; both modify utils along with other files. We merge this batch to production. We then realize PR1 introduced a bug. What do we do?

1. revert PR2 (which was merged last) and then PR1. Essentially we'd revert everything up until the offending PR;
2. revert PR1 (which is second to last in the history). This could be risky if PR2 already counts on some logic from PR1;
3. revert the entire batch. This would cause quite a delay and would require contributors to re-open a lot of PRs, so this isn't ideal.

1 sounds like the best compromise to me, but I'm wondering what the other opinions are, so we align on a process.

[overmyheadandbody]

Usually these revert situations come fast and in a hurry, I'd rather have some alignment now as opposed to during a CSO 🙂

[mokimo]

Since Github can handle simple file merges, it should also be able to handle reverts in the same way. We should be able to reuse our current process and not run into conflicts. I agree that we can't rely on should though, I'd say Option 2 first, but if it doesn't work we'll need to do Option 1.

2. [...] This could be risky if PR2 already counts on some logic from PR1;

Since we usually merge everything within a batch at the same time, I'd say its very unlikely that code is built up/relies on eachother within a batch. From what I've seen, it's more changing something on L100 in utils and L800 in utils, without any correlation to eachother

[overmyheadandbody]

I agree, the chances are slim, but they do exist. I personally always rebase before starting a new branch, so if things go quickly, I could potentially rely on something from the current stage batch, potentially unaware of it.

To re-iterate: I think this is a good step forward, but I'd still like to define a process that the team is aware of and can follow. I tried this on my fork - made a change in brick.js, merged it to a branch, started from that branch to make another change to the same line of code, merged it to that branch. Then tried to revert the first change and got "Sorry, this pull request couldn’t be reverted automatically. It may have already been reverted, or the content may have changed since it was merged.". So we either go with option 1 of always reverting everything up until the offending commit or go with option 2 with the mention that the above message can pop up, case in which we need to fall back on option 1.

[mokimo]

Cool, thanks for getting a workable example on your fork!

go with option 2 with the mention that the above message can pop up, case in which we need to fall back on option 1.

I'd vote for this and see keep our eyes open on how often we need to fall back to option 1