Skip to content

fix(hybrid-cloud): Mark DiscordInteractionsEndpoint as an all silo endpoint#60938

Merged
dashed merged 3 commits into
masterfrom
hybrid-cloud/15-mark-DiscordInteractionsEndpoint-as-all-silo
Dec 1, 2023
Merged

fix(hybrid-cloud): Mark DiscordInteractionsEndpoint as an all silo endpoint#60938
dashed merged 3 commits into
masterfrom
hybrid-cloud/15-mark-DiscordInteractionsEndpoint-as-all-silo

Conversation

@dashed

@dashed dashed commented Nov 30, 2023

Copy link
Copy Markdown
Member

The DiscordInteractionsEndpoint endpoint needs to be marked as @all_silo_endpoint, as it contains code paths that needs to be handled at the control silo. An example would be responding to /help commands; as this interaction doesn't depend on the Organization nor the Integration.

The Discord request parser relies on a valid guild_id (Discord server's id) to fetch the Integration object. If the guild_id is valid, then we route requests to the appropriate region silo. Otherwise they route to the control silo.

I've updated tests to ensure DiscordInteractionsEndpoint is resilient for invalid values of guild_id.

I've also updated DiscordInteractionsEndpoint to be resilient for invalid values of group ids.

@dashed dashed requested a review from a team November 30, 2023 22:39
@dashed dashed self-assigned this Nov 30, 2023
@dashed dashed requested a review from a team as a code owner November 30, 2023 22:39
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 30, 2023
@codecov

codecov Bot commented Nov 30, 2023

Copy link
Copy Markdown

Codecov Report

Merging #60938 (ce78fbd) into master (f0dbad9) will increase coverage by 0.01%.
Report is 13 commits behind head on master.
The diff coverage is 86.95%.

❗ Current head ce78fbd differs from pull request most recent head 183eb00. Consider uploading reports for the commit 183eb00 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #60938      +/-   ##
==========================================
+ Coverage   80.94%   80.96%   +0.01%     
==========================================
  Files        5182     5182              
  Lines      227131   227021     -110     
  Branches    38175    38152      -23     
==========================================
- Hits       183853   183802      -51     
+ Misses      37665    37606      -59     
  Partials     5613     5613              
Files Coverage Δ
src/sentry/integrations/discord/webhooks/base.py 91.48% <100.00%> (ø)
...integrations/discord/webhooks/message_component.py 96.35% <85.71%> (-2.00%) ⬇️

... and 56 files with indirect coverage changes

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 1, 2023
@github-actions

github-actions Bot commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Comment on lines 78 to 79

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not self.group_id:
return self.send_message(INVALID_GROUP_ID)
if not self.group:
return self.send_message(INVALID_GROUP_ID)
if not self.group_id or not self.group:
return self.send_message(INVALID_GROUP_ID)

you could combine these conditions 🏌️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have separate test case for the interactions that are handled on control silo?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiscordMessageComponent relies on guild_id (integration id) and the group id, and needs to run in the region silo. That's why I marked the entire test suite as a region silo test.

We don't anticipate DiscordMessageComponent to ever run in the control silo.

@dashed dashed force-pushed the hybrid-cloud/15-mark-DiscordInteractionsEndpoint-as-all-silo branch from ce78fbd to 183eb00 Compare December 1, 2023 21:46
@dashed dashed force-pushed the hybrid-cloud/15-mark-DiscordInteractionsEndpoint-as-all-silo branch from 183eb00 to 668e273 Compare December 1, 2023 22:39
@dashed dashed enabled auto-merge (squash) December 1, 2023 22:39
@dashed dashed merged commit 2fe6fa4 into master Dec 1, 2023
@dashed dashed deleted the hybrid-cloud/15-mark-DiscordInteractionsEndpoint-as-all-silo branch December 1, 2023 23:00
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants