aminghadersohi opened a new pull request, #44318:
URL: https://github.com/apache/superset/pull/44318
### SUMMARY
The `Check DB migration conflict` workflow is meant to warn the author of an
open PR when the base branch has also changed `superset/migrations`, so they
rebase before two migrations land with the same `down_revision`. It has never
once done so.
It tests each changed file like this:
```js
files.some(x => x.contents_url.includes('/contents/superset/migrations'))
```
The REST API percent-encodes path separators inside `contents_url`, so the
real value is:
```
https://api.github.com/repos/apache/superset/contents/superset%2Fmigrations%2Fversions%2F2026-09-05_00-00_88a01c781622_index_ab_user_lower_username.py?ref=...
```
`/contents/superset/migrations` is not a substring of that, so the predicate
is always `false`. The job still exits 0, so it reports a green check while
doing nothing. A repo-wide search for the comment body it would post
(`"resolving potential db migration conflicts"`) returns **0 results** across
the project's history.
This change matches on `filename` with a prefix test instead — the same
field and the same check that the sibling `enforce-single-migration-head`
workflow already uses correctly:
```js
files.some((f) => f.filename.startsWith('superset/migrations/'))
```
It also drops a duplicate `issue_number` key from the `createComment` call.
The later key shadowed the earlier one, so behaviour is unchanged, but the dead
code path meant it was never exercised.
#### Why this matters right now
`master` currently has two Alembic heads, both declaring `down_revision =
"7e2c9a4f1b83"`:
- `a6c21e5b4d93` → `c7f53d184ea2`
- `88a01c781622`
`superset db upgrade` cannot resolve a single head, so every job that stands
up a database fails. Two PRs branched from the same parent merged within three
hours of each other; neither was rebased onto the other. Both were individually
correct — this is a semantic conflict that only exists once both are on
`master`, and no per-PR check can see it, because each branch has exactly one
head in isolation.
This workflow is the one mechanism designed to cover that window. When the
first of the two PRs merged, it ran, logged `Found 443 open PRs for base branch
"master"`, matched nothing, and posted nothing. The second PR merged two hours
and fifty-three minutes later, with CI results that were by then nine days old.
The repair for the current breakage is #44288 and is deliberately separate
from this PR.
The repository already carries seven `merge_*` revisions, so this situation
recurs.
### TESTING INSTRUCTIONS
The predicate can be checked directly against the two PRs involved. Against
#43939, which added one migration:
```bash
# current behaviour — the predicate that shipped
gh api repos/apache/superset/pulls/43939/files \
--jq '[.[] | select(.contents_url |
contains("/contents/superset/migrations"))] | length'
# => 0
# after this change
gh api repos/apache/superset/pulls/43939/files \
--jq '[.[] | select(.filename | startswith("superset/migrations/"))] |
length'
# => 1
```
End to end, the workflow fires on a push to `master` touching
`superset/migrations/**` and comments on open PRs that also touch that path.
That can be exercised on a fork by pushing a migration to the fork's default
branch with such a PR open.
`pre-commit run` passes on the changed file, including `check yaml` and the
`zizmor` GitHub Actions security audit.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
#### Note on volume, and a scope question for the reviewer
This restores the workflow's original intent without changing it, so it is
worth being explicit that the intent is coarse: it comments whenever an open PR
touches `superset/migrations/`, not only when that PR would actually collide.
There are currently **20 open PRs touching `superset/migrations/`** out of 457
open against `master`, and the workflow has triggered 8 times in the last three
weeks. Un-deadening it will therefore generate real comment volume, most of it
on PRs whose `down_revision` would not have conflicted.
I kept this PR to the bug so the behaviour change is reviewable on its own.
If maintainers would rather it were precise before it starts commenting, the
natural follow-up is to compare the PR's new `down_revision` values against
those just pushed to `master` and comment only on a genuine shared-parent
collision. Happy to fold that in here instead if that is preferred.
#### A gap this does not close
This workflow is a warning, not a gate. The blocking check,
`enforce-single-migration-head`, does evaluate the correct state —
`actions/checkout` resolves `refs/pull/N/merge`, so it sees the PR merged into
its base, not the branch alone. Its limitation is timing rather than logic:
`pull_request` fires on `synchronize`, `opened`, `reopened` and
`ready_for_review`, and movement of the base branch is none of those. A PR that
passed the check keeps that green status however far `master` advances
underneath it, and the check does not re-run before merge unless the branch is
pushed.
Closing that properly is a repository-settings decision rather than a
workflow change — requiring branches to be up to date before merging, or
adopting a merge queue so the check runs against the actual post-merge state at
merge time. Flagging it here for maintainers; it is out of scope for this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
