-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(seer onboarding): make on_command_phrase CR trigger always on #105364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
43317f6
ccd9939
042b1fa
326e24e
f831fcf
507ea1f
0d21203
a98b528
11a8252
d70def4
759c83b
77c80af
945e00e
10d0a0d
a9c9859
79d8a53
4bd87f0
9c02bc8
512f32c
ec2d0c5
85de775
ba9923f
ee73e39
7b60114
9e64f0f
9f017a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| # Generated by Django 5.2.8 on 2025-12-19 23:11 | ||
|
|
||
| from django.db import migrations | ||
| from django.db.backends.base.schema import BaseDatabaseSchemaEditor | ||
| from django.db.migrations.state import StateApps | ||
|
|
||
| from sentry.new_migrations.migrations import CheckedMigration | ||
|
|
||
|
|
||
| def backfill_on_command_phrase_trigger( | ||
| apps: StateApps, schema_editor: BaseDatabaseSchemaEditor | ||
| ) -> None: | ||
| """Backfill on_command_phrase code review trigger for all existing RepositorySettings and Organization defaultCodeReviewTriggers.""" | ||
| RepositorySettings = apps.get_model("sentry", "RepositorySettings") | ||
| OrganizationOption = apps.get_model("sentry", "OrganizationOption") | ||
|
|
||
| # Backfill RepositorySettings | ||
| settings_to_update = [] | ||
|
|
||
| for setting in RepositorySettings.objects.all(): | ||
| triggers = list(setting.code_review_triggers or []) | ||
| if "on_command_phrase" not in triggers: | ||
| setting.code_review_triggers = triggers + ["on_command_phrase"] | ||
| settings_to_update.append(setting) | ||
|
|
||
| if settings_to_update: | ||
| RepositorySettings.objects.bulk_update(settings_to_update, ["code_review_triggers"]) | ||
|
|
||
| # Backfill Organization defaultCodeReviewTriggers | ||
| org_options_to_update = [] | ||
|
|
||
| for org_option in OrganizationOption.objects.filter(key="sentry:default_code_review_triggers"): | ||
| triggers = list(org_option.value or []) | ||
| if "on_command_phrase" not in triggers: | ||
| org_option.value = triggers + ["on_command_phrase"] | ||
| org_options_to_update.append(org_option) | ||
|
|
||
| if org_options_to_update: | ||
| OrganizationOption.objects.bulk_update(org_options_to_update, ["value"]) | ||
|
|
||
|
|
||
| class Migration(CheckedMigration): | ||
| # This flag is used to mark that a migration shouldn't be automatically run in production. | ||
| # This should only be used for operations where it's safe to run the migration after your | ||
| # code has deployed. So this should not be used for most operations that alter the schema | ||
| # of a table. | ||
| # Here are some things that make sense to mark as post deployment: | ||
| # - Large data migrations. Typically we want these to be run manually so that they can be | ||
| # monitored and not block the deploy for a long period of time while they run. | ||
| # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to | ||
| # run this outside deployments so that we don't block them. Note that while adding an index | ||
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = True | ||
|
|
||
| dependencies = [ | ||
| ("sentry", "1015_backfill_self_hosted_sentry_app_emails"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython( | ||
| backfill_on_command_phrase_trigger, | ||
| reverse_code=migrations.RunPython.noop, | ||
| hints={"tables": ["sentry_repositorysettings", "sentry_organizationoptions"]}, | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| from dataclasses import dataclass | ||
| from enum import StrEnum | ||
| from typing import Any | ||
|
|
||
| from django.contrib.postgres.fields.array import ArrayField | ||
| from django.db import models | ||
|
|
@@ -51,6 +52,15 @@ class Meta: | |
|
|
||
| __repr__ = sane_repr("repository_id", "enabled_code_review") | ||
|
|
||
| def save(self, *args: Any, **kwargs: Any) -> None: | ||
| # Ensure ON_COMMAND_PHRASE is always a trigger | ||
| triggers = list(self.code_review_triggers or []) | ||
| if CodeReviewTrigger.ON_COMMAND_PHRASE.value not in triggers: | ||
| triggers.append(CodeReviewTrigger.ON_COMMAND_PHRASE.value) | ||
| self.code_review_triggers = triggers | ||
|
|
||
| super().save(*args, **kwargs) | ||
|
Comment on lines
+57
to
+62
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How frequent are writes to this table? There will be a small time window (a few minutes) between your backfill running and this code being deployed to all servers. Any rows created in that time window will not have
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I expect write freq to be low but if there's no downside to doing it post-deploy, I'll just do so for peace of mind! |
||
|
|
||
| def get_code_review_settings(self) -> CodeReviewSettings: | ||
| """Return code review settings for this repository.""" | ||
| triggers = [CodeReviewTrigger(t) for t in self.code_review_triggers] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to remove this at some point? With this logic as written you're no longer able to turn off the ON_COMMAND_PHRASE trigger right? So presumably we're going to remove this trigger completely (make it no longer configurable), at which point this validation can go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a more broad note in this regard as a whole PR review comment.