feat(ci): Check migrations exist #106253
feat(ci): Check migrations exist #106253
Conversation
If migrations get added, make sure that there's an associated migration lockfile.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| - name: Check if lockfile was updated | ||
| run: | | ||
| if ! git diff --name-only origin/master HEAD | grep -q "migrations_lockfile.txt"; then |
There was a problem hiding this comment.
Bug: The migration-lockfile-guard job uses a git diff command that works for pull requests but will always fail on a push to the master branch, blocking the CI pipeline.
Severity: CRITICAL
Suggested Fix
Restrict the migration-lockfile-guard job to only run on pull_request events. Alternatively, modify the script to use the correct Git references for a push event, such as comparing HEAD with ${{ github.event.before }} to get the correct diff.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/backend.yml#L329
Potential issue: The `migration-lockfile-guard` job in the `backend.yml` workflow is
configured to run on both `pull_request` and `push` events to the `master` branch.
However, the command it uses, `git diff --name-only origin/master HEAD`, is only
effective in a pull request context. When a commit is pushed to `master`, both `HEAD`
and `origin/master` point to the same commit, resulting in an empty diff. Consequently,
the subsequent `grep` command fails to find the `migrations_lockfile.txt` file, causing
the script to exit with an error and fail the CI build on the `master` branch. This will
block deployments that include database migrations.
Did we get this right? 👍 / 👎 to inform future reviews.
.github/workflows/backend.yml
Outdated
| echo "::error::Migration files were added but migrations_lockfile.txt was not updated" | ||
| echo "::error::Run: sentry django makemigrations -n <migration_name>" |
There was a problem hiding this comment.
Are people not doing this? Or is this a safeguard against LLMs?
There was a problem hiding this comment.
I think they forgot to commit the migrations file
There was a problem hiding this comment.
I have changed the wording.
| fi | ||
|
|
||
| migration-lockfile-guard: | ||
| if: github.event_name == 'pull_request' && needs.files-changed.outputs.migrations_added == 'true' |
There was a problem hiding this comment.
We will only run this check on pull requests.
| migrations_added: | ||
| - added: | ||
| - 'src/sentry/**/migrations/*' | ||
| - ':!src/sentry/**/migrations/__init__.py' |
There was a problem hiding this comment.
Exclusion pattern uses unsupported git pathspec syntax
Medium Severity
The exclusion pattern :!src/sentry/**/migrations/__init__.py uses git pathspec syntax, which is not supported by dorny/paths-filter. This action uses picomatch for glob matching, which doesn't recognize :! as an exclusion operator. The pattern will have no effect, so adding __init__.py files to migration directories will still incorrectly trigger the migration-lockfile-guard check. Other patterns in this file correctly use picomatch extglob syntax like !(tests)/**/*.py for exclusions.
This commit adds a test field to UptimeSubscription model without generating the required migration. This is intentional to test whether existing CI checks catch missing migrations for model changes. Related to discussion in #discuss-backend about PR #106253. Co-authored-by: armenzg <armenzg@sentry.io>
alexsohn1126
left a comment
alexsohn1126
left a comment
There was a problem hiding this comment.
The issue with my PR (https://github.com/getsentry/getsentry/pull/19052) was that I forgot to add the migration itself as well as the lockfile.
maybe we should check if there are new models that has been created, and check whether migrations & lockfile is also updated?
| migrations_added: | ||
| - added: | ||
| - 'src/sentry/**/migrations/*' | ||
| - ':!src/sentry/**/migrations/__init__.py' |
There was a problem hiding this comment.
Why do you need to exclude init files? They are generally empty and don't change.
If migrations get added, make sure that there's an associated migration lockfile.