feat(code_review): Support options & white listed orgs #106205
Conversation
suejung-sentry
left a comment
There was a problem hiding this comment.
I'm okay with approving this to be merged if you're feeling confident in the logic but my feeble brain can't handle all the boolean logic to uncover any bugs lol so suggested some renaming to help, or we can just get this merged since we're going to rip it out in a matter of days anyway.
| assert not mock_request.called | ||
| assert mock_request.call_count == 1 | ||
| pr_call = mock_request.call_args | ||
| assert pr_call[1]["path"] == "/v1/automation/overwatch-request" |
There was a problem hiding this comment.
I see this was the bug you found because since we have not turned on any flags it should still go to overwatch but I am struggling for what exactly was flipped. Was it a missing None handling somewhere?
There was a problem hiding this comment.
The tests in this class need to be reviewed since we always call the task from the handlers rather the way we do it in this class:
process_github_webhook_event._func(
github_event=GithubWebhookType.PULL_REQUEST,
event_payload=event_payload,
enqueued_at_str=self.enqueued_at_str,
)
I will look into it tomorrow.
| } | ||
|
|
||
|
|
||
| def get_github_events_to_forward_overwatch() -> set[GithubWebhookType]: |
There was a problem hiding this comment.
Dropping this to simplify the code.
| target_commit_sha=target_commit_sha, | ||
| trigger=SeerCodeReviewTrigger.ON_COMMAND_PHRASE, | ||
| ) | ||
| record_webhook_enqueued(github_event, github_event_action) |
There was a problem hiding this comment.
Now tracked inside of of schedule_task.
| """ | ||
| status = "success" | ||
| should_record_latency = True | ||
| option_key = get_webhook_option_key(github_event) |
There was a problem hiding this comment.
This whole block is unnecessary since we check at the handlers level.
| assert not mock_request.called | ||
| assert mock_request.call_count == 1 | ||
| pr_call = mock_request.call_args | ||
| assert pr_call[1]["path"] == "/v1/automation/overwatch-request" |
There was a problem hiding this comment.
The tests in this class need to be reviewed since we always call the task from the handlers rather the way we do it in this class:
process_github_webhook_event._func(
github_event=GithubWebhookType.PULL_REQUEST,
event_payload=event_payload,
enqueued_at_str=self.enqueued_at_str,
)
I will look into it tomorrow.
suejung-sentry
left a comment
There was a problem hiding this comment.
This makes sense to me. Can we turn OFF DE in production before we merge this in (at least for issue_comment)?
I'm pretty sure the issue_comment flow in the new stuff is broken so we need that to continue to go through overwatch or have a bad day haha
| except ValueError: | ||
| return False | ||
| # Installation events are always forwarded | ||
| is_installation = event_type_enum in _INSTALLATION_EVENTS |
There was a problem hiding this comment.
We should not be forwarding installation events. I think this flow is not hooked up upstream. Thread here . If you leave this in it will probably just be a no-op so that's ok too
There was a problem hiding this comment.
I can handle that on a different PR.
As of today, we were still forwarding them:
We don't need to turn it off first. |
I tested out the prevent ai pods for sentry --> seer and looks good. This PR cuts us over wholesale. (Note we've still got more stuff upstream deciding if this flow is even invoked so not live for customers yet) This depends on this [PR](#106205) merging first. --------- Co-authored-by: Armen Zambrano G. <44410+armenzg@users.noreply.github.com>
In #105844 (while we were debugging
s4s2) we switched to using whitelisted GitHub orgs rather than options.Now that we've made progress in s4s2 we can add using options back again. We will drop whitelisted orgs in the future.