-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(code-review): Consolidate code review checks #105561
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
armenzg
left a comment
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.
Well done! 🎉
Besides tests, could you write a ticket to help other parts of Sentry use your logic? (or see how easy it is to switch over) That way we have a centralized place and we prevent unintentional regressions.
Until we officially freeze the code review beta list, consider anyone with the feature toggle ON as part of the beta too. Note that we'll replace this with the logic [here](#105561) shortly, but wanted to unblock this.
d9e3314 to
9a8da6d
Compare
Here's a ticket CW-111. I think easiest is to do a hard cutover to this new flow path and the old one within overwatch can just die on the vine, but we can see how the timing works out. |
| except OrganizationContributors.DoesNotExist: | ||
| metrics.incr( | ||
| "overwatch.code_review.contributor_not_found", | ||
| tags={"organization_id": organization.id, "repository_id": repository_id}, |
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.
Note that when I moved this over I removed the tag for repository id as I thought it would add too much cardinality on the metric. Will connect with @ajay-sentry if that is going to be a problem..
| organization: The Sentry organization that the webhook event belongs to | ||
| repo: The repository that the webhook event is for | ||
| **kwargs: Additional keyword arguments including integration | ||
| integration: The GitHub integration |
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.
Looks like we need integration for the preflight billing check
| pr_author_external_id=get_pr_author_id(event), | ||
| ).check() | ||
| if not preflight.allowed: | ||
| # TODO: add metric |
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.
Will add a metric consistent with any new standards in this PR - https://github.com/getsentry/sentry/pull/105587/files
|
|
||
| @patch("sentry.seer.code_review.webhooks.task.process_github_webhook_event") | ||
| @with_feature({"organizations:gen-ai-features"}) | ||
| def test_check_run_runs_when_code_review_beta_flag_disabled_but_pr_review_test_generation_enabled( |
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.
this testcase is covered in the preflight class tests
| } | ||
|
|
||
|
|
||
| def get_pr_author_id(event: Mapping[str, Any]) -> str | None: |
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.
armenzg
left a comment
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.
Good from my POV. Feel free to merge.
You may want input from someone familiar with billing for the tests you added.
| integration_id=integration.id if integration else None, | ||
| pr_author_external_id=get_pr_author_id(event), | ||
| ).check() | ||
| if not preflight.allowed: |
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.
Great to see it being handled in the main handler function 🦾 .
44295f1 to
d46d0e1
Compare
Thanks, I'll connect with @ajay-sentry when he's back on Wednesday. The billing checks aren't live until we GA so should be OK to merge in the meantime |
| def _check_billing(self) -> DenialReason: | ||
| return None | ||
|
|
||
| # TODO: Once we're ready to actually gate billing (when it's time for GA), uncomment this |
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.
Note that I updated this to always let the billing check go through for now since it's not turned on for the overwatch flow yet either (just logged here). This will be turned on with GA.
de5360b to
67c1c83
Compare
| if (user_id := event.get("user", {}).get("id")) is not None: | ||
| return str(user_id) | ||
|
|
||
| # Check sender.id (for check_run events). Sender is the user who triggered the event |
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.
I'll confirm with @ajay-sentry / Product who/what we want to increment for this webhook event. It's possible they would want this one to be "always free"
67c1c83 to
ddea878
Compare
ddea878 to
3de8ad8
Compare
armenzg
left a comment
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 of this code I did not merge into my PR since it did not seem necessary. Please let me know if I got it wrong!
| github_integration: Integration | None = None | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_billing_quota(self) -> Generator[None]: |
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.
When I merged this code into #105673 I did not port this fixture.
| self.organization.update_option("sentry:enable_pr_review_test_generation", True) | ||
|
|
||
| # Setup billing data | ||
| self.github_integration = self.create_github_integration() |
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.
I did not merge this code.
| self.organization.update_option("sentry:enable_pr_review_test_generation", True) | ||
|
|
||
| # Setup billing data | ||
| self.github_integration = self.create_github_integration() |
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.
I did not merge this.
| repo_id = int(self.event_dict["repository"]["id"]) | ||
|
|
||
| integration = self.create_github_integration() | ||
| integration = self.github_integration or self.create_github_integration() |
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.
Same.
| self.mock_seer.assert_not_called() | ||
|
|
||
| @with_feature({"organizations:gen-ai-features"}) | ||
| def test_runs_when_code_review_beta_flag_disabled_but_pr_review_test_generation_enabled( |
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.
Merged as this:
sentry/tests/sentry/seer/code_review/webhooks/test_issue_comment.py
Lines 100 to 110 in 463c021
| def test_runs_when_code_review_beta_flag_disabled_but_pr_review_test_generation_enabled( | |
| self, | |
| ) -> None: | |
| """Test that processing runs with gen-ai-features flag alone when org option is enabled.""" | |
| with self.code_review_setup(features={"organizations:gen-ai-features"}), self.tasks(): | |
| event = self._build_issue_comment_event(f"Please {SENTRY_REVIEW_COMMAND} this PR") | |
| response = self._send_issue_comment_event(event) | |
| assert response.status_code == 204 | |
| self.mock_seer.assert_called_once() |
Consolidate all the different checks for code review to one spot so we can use it in our new webhook flow.
Note that I left the billing check off for now and will check with the team on how best to transition it on for GA
Closes CW-88