-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
db: update webhookpayload #93855
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
db: update webhookpayload #93855
Conversation
I add the destination_type field, make the region name nullable and add a constraint so if the destination_type == SENTRY_REGION then the region_name must be set to a value. These are the equivalent database changes for a following change that will implement webhook forwarding to Codecov using the hybrid cloud webhook forwarding system. Webhooks meant to be forwarded to Codecov will have differing logic that runs to forward them which is why I'm adding the destination_type field so we can distinguish them.
This PR has a migration; here is the generated SQL for for --
-- Add field destination_type to webhookpayload
--
ALTER TABLE "hybridcloud_webhookpayload" ADD COLUMN "destination_type" varchar DEFAULT 'sentry_region' NOT NULL;
--
-- Alter field region_name on webhookpayload
--
ALTER TABLE "hybridcloud_webhookpayload" ALTER COLUMN "region_name" DROP NOT NULL;
--
-- Create constraint webhookpayload_region_name_not_null on model webhookpayload
--
ALTER TABLE "hybridcloud_webhookpayload" ADD CONSTRAINT "webhookpayload_region_name_not_null" CHECK (("destination_type" = 'sentry_region' AND "region_name" IS NOT NULL)) NOT VALID;
ALTER TABLE "hybridcloud_webhookpayload" VALIDATE CONSTRAINT "webhookpayload_region_name_not_null"; |
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.
You also need to make the changes to the model in this pr
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93855 +/- ##
===========================================
+ Coverage 65.57% 88.05% +22.47%
===========================================
Files 10367 10337 -30
Lines 599732 598683 -1049
Branches 23351 23181 -170
===========================================
+ Hits 393275 527151 +133876
+ Misses 205982 71074 -134908
+ Partials 475 458 -17 |
destination_type: DestinationType, | ||
region: str | None, |
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.
Probably we can exclude these changes. I think that the non-optional parameter will break existing calls
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.
Yes this will fail as it currently stands. I would leave this change in your other set of changes.
@wedamija @markstory this PR is good to merge? |
constraints = [ | ||
models.CheckConstraint( | ||
check=Q(destination_type=DestinationType.SENTRY_REGION) | ||
& Q(region_name__isnull=False), | ||
name="webhookpayload_region_name_not_null", | ||
), | ||
] |
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.
this introduces a new warning:
/Users/asottile/workspace/sentry/src/sentry/hybridcloud/models/webhookpayload.py:83: RemovedInDjango60Warning: CheckConstraint.check is deprecated in favor of `.condition`.
models.CheckConstraint(
surprised this didn't get caught in CI but please fix 🙏
I add the destination_type field, make the region name nullable and add a constraint so if the destination_type == SENTRY_REGION then the region_name must be set to a value.
These are the equivalent database changes for a following change that will implement webhook forwarding to Codecov using the hybrid cloud webhook forwarding system. Webhooks meant to be forwarded to Codecov will have differing logic that runs to forward them which is why I'm adding the destination_type field so we can distinguish them.