-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): filter out non-alertable and broken sentry apps in available actions endpoint #101866
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #101866 +/- ##
=========================================
Coverage 80.98% 80.98%
=========================================
Files 8721 8728 +7
Lines 387858 388495 +637
Branches 24548 24548
=========================================
+ Hits 314098 314623 +525
- Misses 73415 73527 +112
Partials 345 345 |
| prepared_component = prepare_ui_component( | ||
| installation=context.installation, | ||
| component=context.component, | ||
| ) |
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.
Does this component preparation code make any network requests at all?
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.
Summarizing a conversation we had in Slack:
This potentially calls the downstream Sentry App's endpoint, which can add a ton of additional latency, or fallibility.
This is ok for now, but we should consider moving this call into its own API, and letting the UI make this call on-demand to retrieve just the UI component that it needs when the user selects the Sentry app as a target and has to configure it.
Additionally, this call is repeated in the serializer below, so we're removing that other call and hoisting it here for now while @ameliahsu works on the API and UI changes.
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
fixes 2 bugs that were causing the available actions endpoint to return more results than expected:
is_alertable, causing the webhook action to return extra SentryAppServices that are not alertableprepare_ui_componenton the sentry app components, causing us to return broken sentry apps that fail component preparation (see below)sentry/src/sentry/sentry_apps/models/sentry_app_installation.py
Lines 255 to 256 in da98041