feat(code_review): Support options & white listed orgs #106205
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
suejung-sentry
left a comment
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.
src/sentry/seer/code_review/utils.py
Outdated
| return None | ||
|
|
||
|
|
||
| def should_proceed(github_event: GithubWebhookType, event_payload: Mapping[str, Any]) -> bool: |
There was a problem hiding this comment.
naming nit alert
should_forward_to_seer ?
| 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.
armenzg
changed the title
| } | ||
|
|
||
|
|
||
| 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.
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.