require PRs to pass against master before getting merged

[jrhizor]

We've seen several problems lately where master is broken because conflicting changes on master and some pr don't show up in a merge conflict.

Latest example:
#6975 was passing on the PR build, was merged but immediately caused failures due to changes on master that weren't part of the PR builds, which broke Airbyte registry access and required #7565 to fix.

[jrhizor]

yet another example: https://github.com/airbytehq/airbyte/runs/4111929059?check_suite_focus=true

[noahkawasakigoogle]

I'm curious about this issue existing, it seems like it could be a pretty big problem. Especially when Airbyte scales engineering up and more and more PRs are merging each week.

Do Airbyte pull requests not build jobs/tests using the merge candidate commit but instead build literally the branch itself?

If thats true, there's a way to change this behavior to run builds against the current master branch with the developer branch merged together (merge candidate commit) by using the event pull_request instead of push.

It might be as simple as changing the workflow file to this:

on: 
  pull_request

Would probably require some additional testing/verification on certain things though

[timroes]

Passing PRs against master sounds a good first step. Unfortunately there will still be one potential conflict case. If you let your PR hang around for too long after the last CI build (e.g. because reviewing takes some time), it could already be in a state causing problems merging into master, and it won't be detected because the CI simply doesn't run again anymore since there have been no changes triggering it.

In my previous projects we automatically invalidated PRs that have not been rebased/merged from master longer than 48h. That would enforce everyone to make sure their PR is also based on a recent master branch, plus makes sure that even after CI ran and passed it, it would automatically invalidate if it's staying around for too long without activity. I feel this was a good solution reducing conflicts (on a repo with roughly 30-50 PRs merged per day).

[davinchia]

We've discussed this several times and decided that this is something that will slow down dev speed enough that we don't want to enact this yet. I'm closing this or now.