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://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?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.
|
|
||
| - 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 hardcodes origin/master for its git diff. This will cause incorrect validation for pull requests targeting branches other than master.
Severity: HIGH
Suggested Fix
Replace the hardcoded origin/master with a dynamic reference to the pull request's base branch. Use origin/${{ github.base_ref }} or origin/${{ github.event.pull_request.base.ref }} to ensure the diff is always against the correct target branch.
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 is configured to run on pull
requests targeting any branch. However, the script at line 329 uses `git diff
--name-only origin/master HEAD` to check for changes. This hardcoded reference to
`origin/master` is incorrect when a pull request's base branch is not `master` (e.g., a
release branch). This will cause the migration lockfile check to compare against the
wrong set of changes, potentially leading to false positives or false negatives, and
allowing migrations to be merged without a corresponding lockfile update.
Did we get this right? 👍 / 👎 to inform future reviews.
| 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>
If migrations get added, make sure that there's an associated migration lockfile.