Prevent divergent migrations from being merged by concurrent, not-rebased PRs

[sarayourfriend]

trafficstars

Problem

See https://github.com/WordPress/openverse-api/pull/810 and the description for the issue.

Description

We need some way to prevent this.

Here are some suggestions I left in this comment, copied below: https://github.com/WordPress/openverse-api/pull/810#issuecomment-1191503799

How can we prevent this? Is there any way we can ensure PRs with migrations are always rebased before merging? Could we run an action triggered by pushes that introduce new migrations using https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-workflow-only-when-a-push-affects-specific-files and the iterate through all open, non-draft PRs and request changes on them if there is a migration in them?

We'd also need a similar job in the CI + CD workflow to check for migration conflicts against main any time the PR is pushed to.

Marked as high due to the risk it introduces.

Implementation

• [ ] 🙋 I would be interested in implementing this feature.

[dhruvkb]

Referencing #812 (duplicate) as an alternative way to phrase the issue.

[ritesh-pandey]

Can I add something to the discussion?

Can we say that in an ideal migration tree, there should be only one leaf node?

If yes, we should think of running automated tests which detect this. Wring a custom Django command can be one way to achieve this.

A Django command which checks for the state of migration graph. If the command finds more than one leaf node, it exits with a certain exit code. This command can be integrated in CI steps as well.

[sarayourfriend]

That's correct. I think our existing CI checks will do that already, save for the fact that they need to be run against a rebased version of the branch for them to detect the fact that a branch will merge in a second branch of migations. This happens when two people open branches that independently add migrations with the same number and then merge them sequentially without rebasing the second branch after the first is merged.

@dhruvkb and I were chatting in DMs about this and we think that the pull_request_target: [synchronize] workflow trigger would be the right one to use to rebase the branch against main and then run the CI check. We can skip this for now for branches that don't introduce migrations, that way we don't run the risk of eating up a ton of our GitHub worker allowances. We could also skip it for PRs that are otherwise unmergeable (not enough approvals, not checks not passing), but then we'd need to make sure we also run the rebase check whenever those things are covered.

[dhruvkb]

Adding more context, GitHub has a built-in feature to ensure branches are up-to-date before they can be merged (it's a checkbox called "Require branches to be up to date before merging" under branch protection rules) but it's either all-in (which can be annoying for safe branches) or switched off (which brings us to this issue).

GitHub also does not provide a workflow event that runs just before a merge where we could validate the PR against main and block the merge in case of a migration fault.

[sarayourfriend]

The require branches to be up-to-date before merging is useful but it is kind of annoying and a bit of a kludge in that it enforces that universally. Even if you just update a markdown document, you'd have to rebase your branch if main changed and wait for CI to run all over again :confused: Of course, we could also optimize CI to only change when certain files change, but that's yet another issue.

I think the synchronize event is the right time to do it. It's proactive, we can make the action required for PRs and it will block. Waiting for when the person hits the merge button is annoying and frustrating if it fails when we could have caught the issue proactively anyway.

[dhruvkb]

The synchronize event is only fired when commits are pushed to a PR. It's also the default activity type for workflows running on PRs (see first "Note" in this section). Maybe I'm not understanding currently but to me synchronise does not seem to be the right event.

[sarayourfriend]

That's the pull_request event. I'm suggesting using the pull_request_target event which runs when the PR's target or base branch is updated, unless I've misunderstood that event trigger.

[sarayourfriend]

Unassigning myself here. The PR linked above adds a warning, but I think we can do better to prevent this by running actual automated checks for migration conflicts. I won't have time to work on this before I go on holiday.

[zackkrida]

Lowering the priority to medium, this is important but is an internal improvement that shouldn't block other development.