ref(integrations): integration webhook endpoint ABC #82739
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82739 +/- ##
==========================================
+ Coverage 87.48% 87.50% +0.01%
==========================================
Files 9410 9411 +1
Lines 536794 536847 +53
Branches 21049 21049
==========================================
+ Hits 469640 469752 +112
+ Misses 66785 66726 -59
Partials 369 369 |
| self.add_response() | ||
|
|
||
| self.get_error_response( | ||
| extra_headers=dict(HTTP_AUTHORIZATION="JWT anexampletoken"), | ||
| extra_headers=dict(HTTP_AUTHORIZATION="JWT " + self.jwt_token_cdn()), |
There was a problem hiding this comment.
needed to add this because checking auth was moved before checking the body
| """ | ||
| Webhook hit by Jira whenever someone uninstalls the Sentry integration from their Jira instance. | ||
| """ | ||
|
|
||
| def post(self, request: Request, *args, **kwargs) -> Response: | ||
| def authenticate(self, request: Request, **kwargs) -> Any: |
There was a problem hiding this comment.
ideally authenticating the incoming request would be on its own (without any return value and raising exceptions if an error occurred), but the get_integration_from_jwt function decodes the token, fetches the integration, and verifies the token (possibly using information from integration.metadata), so it's not easy to decouple
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
||
| self.log_extra = {} |
There was a problem hiding this comment.
While this is helpful for consistent logging, do we always need to log this full context, or is it ok to have the subclass methods decide what should be logged?
There was a problem hiding this comment.
the only logs that are by default is request_method and request_path, and each subclass adds more into self.log_extra as necessary. what do you mean by full context here?
There was a problem hiding this comment.
Just that we're always appending additional data into this context object. If these are the only 2 places we're logging though, it's fine.
There was a problem hiding this comment.
oh wait i think i have to double check that this is actually being initialized each time. maybe i reinitialize when dispatch is called?
cathteng
force-pushed
the
cathy/integrations/webhook-endpoint-abc
branch
from
cc43759 to
4e769d5
Compare
| @csrf_exempt | ||
| def dispatch(self, request, *args, **kwargs): | ||
| response = super().dispatch(request, *args, **kwargs) | ||
| self.log_extra = {"request_method": self.request.method, "request_path": self.request.path} |
There was a problem hiding this comment.
@GabeVillalobos i think i need to do this because idk if the class is initialized if we link it to the url via Endpoint.as_view()
There was a problem hiding this comment.
i don't think the class gets re-initialized on every API call so i would need to reset the log extra manually here
|
This issue 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 remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Begin tying together the various integration webhook endpoints. Jira differs from the others in that it has an API for each event. It still has the same basic layout of:
So the first 2 can be encapsulated in a shared base class.