-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
chore(sentry apps): Audit & use new errors for external req processes & endpoints #83077
base: master
Are you sure you want to change the base?
chore(sentry apps): Audit & use new errors for external req processes & endpoints #83077
Conversation
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
some nits and a question
@@ -45,7 +44,10 @@ def post(self, request: Request, installation) -> Response: | |||
data = request.data.copy() | |||
|
|||
if not {"groupId", "action", "uri"}.issubset(data.keys()): | |||
return Response(status=400) | |||
raise SentryAppError( | |||
ValidationError('"groupId", "action", and "uri" must be present in request'), |
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.
nit: i would either use group_id
or groupID
, same everywhere else
f"Invalid response format from sentry app {self.sentry_app} when linking issue" | ||
raise SentryAppIntegratorError( | ||
ValidationError( | ||
f"Invalid response format from sentry app {self.sentry_app} when linking issue" |
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.
lets use integration instead of sentry app, not sure if we mention sentry app externally in docs.
f"Something went wrong while getting SelectFields from {self.sentry_app.slug}" | ||
raise SentryAppIntegratorError( | ||
APIError( | ||
f"Something went wrong while getting SelectFields from {self.sentry_app.slug}" |
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.
do integrators know what SelectFields is or is it something inside our code?
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.
Maybe ? The most accurate name is Select FormField so we could use that instead
Title
Basically go through all the errors in external request (i.e when we make a req to the third party app for some data) processes and label SentryAppError or SentryAppIntegratorError.
For endpoints I went through and instead of returning a response for errors, we raise the error again and our base class error handler will deal with the error for us.