Migration lock file check #106299
Migration lock file check #106299
Conversation
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 <[email protected]>
|
Cursor Agent can help with this pull request. Just |
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # be associated, this just controls the span sampling. | ||
| trace_sampling = models.BooleanField(default=False, db_default=False) | ||
| # Test field to verify CI catches missing migrations | ||
| test_migration_check = models.CharField(max_length=100, null=True, default=None) |
There was a problem hiding this comment.
Test field without migration should not be merged
High Severity
A test field test_migration_check has been added to the UptimeSubscription model without a corresponding migration file. The comment explicitly states this is a "Test field to verify CI catches missing migrations." This is test/debugging code that should not be merged to production. If deployed, the model would reference a database column that doesn't exist, causing runtime errors when accessing UptimeSubscription records.
wedamija
left a comment
wedamija
left a comment
There was a problem hiding this comment.
Approving to see whether this would let us merge - don't merge it
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| test_migration_check = models.CharField(max_length=100, null=True, default=None) | ||
|
|
||
| objects: ClassVar[BaseManager[Self]] = BaseManager( | ||
| cache_fields=["pk", "subscription_id"], |
There was a problem hiding this comment.
Bug: The UptimeSubscription model was changed by adding the test_migration_check field, but no corresponding database migration was generated.
Severity: CRITICAL
Suggested Fix
Generate a new migration file to reflect the changes in the UptimeSubscription model. This can be done by running the command python manage.py makemigrations uptime and committing the resulting migration file.
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: src/sentry/uptime/models.py#L99
Potential issue: A new field `test_migration_check` has been added to the
`UptimeSubscription` model, but a corresponding database migration file has not been
created. When code attempts to create or update an `UptimeSubscription` instance, the
Django ORM will generate an SQL query that includes the new field. Because the
`test_migration_check` column does not exist in the database schema, the database will
reject the query, leading to a `django.db.utils.ProgrammingError` at runtime.
Did we get this right? 👍 / 👎 to inform future reviews.
This PR introduces a model change to
UptimeSubscriptionwithout generating a corresponding migration file.The purpose is to test if existing CI checks in the
sentryrepo successfully detect the missing migration. This will help determine if additional migration checks (like those in PR #106253) are necessary forsentryor only forgetsentry.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Slack Thread