Skip to content

feat(gh-comments): Queuing Logic #50865

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jun 15, 2023
Merged

feat(gh-comments): Queuing Logic #50865

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9151fa3
feat(gh-comment): Queuing logic
Jun 12, 2023
629fa50
working on tests
Jun 13, 2023
8b88835
syncing
Jun 13, 2023
fd48476
Merge branch 'master' into adas/gh-comment-bot/queuing-logic
Jun 13, 2023
6be211d
Workikng on tests
Jun 13, 2023
56efbb1
troubleshooting tests
Jun 13, 2023
5ccba4b
complete queuing logic
Jun 13, 2023
21f4b3d
Merge branch 'master' into adas/gh-comment-bot/queuing-logic
Jun 13, 2023
17fa05b
resync
Jun 13, 2023
abff1e7
Merge branch 'master' into adas/gh-comment-bot/queuing-logic
Jun 13, 2023
5bd3151
resynced for db model changes
Jun 13, 2023
6a834ab
pr comments applied
Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions src/sentry/tasks/commit_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import logging
from datetime import timedelta
from datetime import datetime, timedelta

from celery.exceptions import MaxRetriesExceededError
from django.utils import timezone
from sentry_sdk import set_tag

from sentry import analytics
from sentry import analytics, features
from sentry.api.serializers.models.release import get_users_for_authors
from sentry.integrations.utils.commit_context import find_commit_context_for_event
from sentry.locks import locks
from sentry.models import Commit, CommitAuthor, Project, RepositoryProjectPathConfig
from sentry.models import (
Commit,
CommitAuthor,
Project,
PullRequest,
Repository,
RepositoryProjectPathConfig,
)
from sentry.models.groupowner import GroupOwner, GroupOwnerType
from sentry.shared_integrations.exceptions import ApiError
from sentry.tasks.base import instrumented_task
Expand All @@ -26,6 +33,24 @@
logger = logging.getLogger(__name__)


def queue_comment_task_if_needed(commit: Commit, group_owner: GroupOwner):
from sentry.tasks.integrations.github.pr_comment import comment_workflow

pr = PullRequest.objects.filter(
organization_id=commit.organization_id, merge_commit_sha=commit.key
)
if (
pr.exists()
and pr[0].date_added >= datetime.now(tz=timezone.utc) - timedelta(days=30)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would pr ever include more than one object? and if so why do we choose pr[0] over the others?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pr is technically a query so we might want to call it pr_query. we could also use get() but get() throws an error if the item doesn't exist or if multiple items exist that match the criteria

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although we should only have one match so we could use get()

and (
not pr[0].pullrequestcomment_set.exists()
or group_owner.group_id not in pr[0].pullrequestcomment_set.get().group_ids
)
):
# TODO: Debouncing Logic
comment_workflow.delay(pullrequest_id=pr[0].id, project_id=group_owner.project_id)


@instrumented_task(
name="sentry.tasks.process_commit_context",
queue="group_owners.process_commit_context",
Expand Down Expand Up @@ -271,6 +296,11 @@ def process_commit_context(
}, # Updates date of an existing owner, since we just matched them with this new event
)

if features.has("organizations:pr-comment-bot", project.organization):
repo = Repository.objects.get(id=commit.repository_id)
if repo is not None and repo.provider == "integrations:github":
queue_comment_task_if_needed(commit, group_owner)

if created:
# If owners exceeds the limit, delete the oldest one.
if len(current_owners) + 1 > PREFERRED_GROUP_OWNERS:
Expand Down
165 changes: 161 additions & 4 deletions tests/sentry/tasks/test_commit_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
from datetime import timedelta
from unittest.mock import patch
from unittest.mock import Mock, patch

import pytest
from celery.exceptions import MaxRetriesExceededError
from django.utils import timezone

from sentry.models import Repository
from sentry.models import PullRequest, PullRequestComment, Repository
from sentry.models.commit import Commit
from sentry.models.groupowner import GroupOwner, GroupOwnerType
from sentry.shared_integrations.exceptions.base import ApiError
from sentry.tasks.commit_context import process_commit_context
from sentry.testutils import TestCase
from sentry.testutils.helpers import with_feature
from sentry.testutils.helpers.datetime import before_now, iso_format
from sentry.testutils.silo import region_silo_test
from sentry.utils.committers import get_frame_paths


@region_silo_test(stable=True)
class TestCommitContext(TestCase):
class TestCommitContextMixin(TestCase):
def setUp(self):
self.project = self.create_project()
self.repo = Repository.objects.create(
Expand Down Expand Up @@ -72,6 +72,9 @@ def setUp(self):
project_id=self.project.id,
)


@region_silo_test(stable=True)
class TestCommitContext(TestCommitContextMixin):
@patch(
"sentry.integrations.github.GitHubIntegration.get_commit_context",
return_value={
Expand Down Expand Up @@ -408,3 +411,157 @@ def after_return(self, status, retval, task_id, args, kwargs, einfo):
)

assert mock_suspect_commits.called


@region_silo_test(stable=True)
@patch(
"sentry.integrations.github.GitHubIntegration.get_commit_context",
Mock(
return_value={
"commitId": "asdfwreqr",
"committedDate": "2023-02-14T11:11Z",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it better to add a date() here so you make sure in tests the commit is always recent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the commit date isn't important in this case. I'd rather just mock a response once here and have the object be fixed.

"commitMessage": "placeholder commit message",
"commitAuthorName": "",
"commitAuthorEmail": "admin@localhost",
}
),
)
@patch("sentry.tasks.integrations.github.pr_comment.comment_workflow.delay")
class TestGHCommentQueuing(TestCommitContextMixin):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does putting all the patches before the class mean that all the patches are applied to the tests inside the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! Saves on code repetition. I think you can override it too if you have one or two tests that want a different patch

def setUp(self):
super().setUp()
self.pull_request = PullRequest.objects.create(
organization_id=self.organization.id,
repository_id=self.repo.id,
key="99",
author=self.commit.author,
message="foo",
title="bar",
merge_commit_sha=self.commit.key,
date_added=iso_format(before_now(days=1)),
)
self.repo.provider = "integrations:github"
self.repo.save()
self.pull_request_comment = PullRequestComment.objects.create(
pull_request=self.pull_request,
external_id=1,
created_at=iso_format(before_now(days=1)),
updated_at=iso_format(before_now(days=1)),
group_ids=[],
)

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_not_github(self, mock_comment_workflow):
"""Non github repos shouldn't be commented on"""
self.repo.provider = "integrations:gitlab"
self.repo.save()
with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert not mock_comment_workflow.called

def test_gh_comment_feature_flag(self, mock_comment_workflow):
"""No comments on org with feature flag disabled"""
with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert not mock_comment_workflow.called

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_no_pr(self, mock_comment_workflow):
"""No comments on suspect commit with no pr"""
self.pull_request.delete()
with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert not mock_comment_workflow.called

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_org_settings(self, mock_comment_workflow):
"""No comments on org who disabled feature"""
# TODO(Cathy or Aniket): implement once the toggle is merged
pass

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_pr_too_old(self, mock_comment_workflow):
"""No comment on pr that's older than 30 days"""
self.pull_request.date_added = iso_format(before_now(days=31))
self.pull_request.save()

with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert not mock_comment_workflow.called

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_repeat_issue(self, mock_comment_workflow):
"""No comment on a pr that has a comment with the issue in the same pr list"""
self.pull_request_comment.group_ids.append(self.event.group_id)
self.pull_request_comment.save()

with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert not mock_comment_workflow.called

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_create_queued(self, mock_comment_workflow):
"""Task queued if no prior comment exists"""
self.pull_request_comment.delete()

with self.tasks():
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert mock_comment_workflow.called

@with_feature("organizations:pr-comment-bot")
def test_gh_comment_update_queue(self, mock_comment_workflow):
"""Task queued if new issue for prior comment"""

with self.tasks():
assert not GroupOwner.objects.filter(group=self.event.group).exists()
event_frames = get_frame_paths(self.event)
process_commit_context(
event_id=self.event.event_id,
event_platform=self.event.platform,
event_frames=event_frames,
group_id=self.event.group_id,
project_id=self.event.project_id,
)
assert mock_comment_workflow.called