Skip to content

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

Merged
merged 6 commits into from
Jun 25, 2025
Merged

db: update webhookpayload #93855

merged 6 commits into from
Jun 25, 2025

Conversation

joseph-sentry
Copy link
Contributor

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.

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.
@joseph-sentry joseph-sentry requested a review from a team as a code owner June 18, 2025 20:18
@joseph-sentry joseph-sentry requested a review from wedamija June 18, 2025 20:19
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 18, 2025
Copy link
Contributor

This PR has a migration; here is the generated SQL for src/sentry/hybridcloud/migrations/0022_webhook_payload_update.py

for 0022_webhook_payload_update in hybridcloud

--
-- 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";

Copy link
Member

@wedamija wedamija left a 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

Copy link

codecov bot commented Jun 20, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/hybridcloud/tasks/deliver_webhooks.py 80.00% 2 Missing ⚠️
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     

Comment on lines 117 to 118
destination_type: DestinationType,
region: str | None,
Copy link
Member

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

Copy link
Member

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.

@joseph-sentry
Copy link
Contributor Author

@wedamija @markstory this PR is good to merge?

@joseph-sentry joseph-sentry merged commit 045414e into master Jun 25, 2025
65 checks passed
@joseph-sentry joseph-sentry deleted the joseph/webhook-migration branch June 25, 2025 19:41
Comment on lines +82 to +88
constraints = [
models.CheckConstraint(
check=Q(destination_type=DestinationType.SENTRY_REGION)
& Q(region_name__isnull=False),
name="webhookpayload_region_name_not_null",
),
]
Copy link
Member

@asottile-sentry asottile-sentry Jun 26, 2025

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 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants