From 2e0f06fd298945789a8142ccf9f04eec64a83d3b Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Thu, 16 May 2024 11:57:46 -0700 Subject: [PATCH 1/4] abstraction for triggering merged PR comments workflow --- src/sentry/integrations/github/client.py | 19 ++++++++++ src/sentry/tasks/commit_context.py | 38 +++++++------------ .../sentry/integrations/github/test_client.py | 34 +++++++++++++++++ 3 files changed, 67 insertions(+), 24 deletions(-) diff --git a/src/sentry/integrations/github/client.py b/src/sentry/integrations/github/client.py index 4dd56974f98ea3..2f945b305ce525 100644 --- a/src/sentry/integrations/github/client.py +++ b/src/sentry/integrations/github/client.py @@ -224,6 +224,25 @@ def get_commit(self, repo: str, sha: str) -> Any: """ return self.get_cached(f"/repos/{repo}/commits/{sha}") + def get_merge_commit_sha_from_commit(self, repo: str, sha: str) -> str | None: + """ + Get the merge commit sha from a commit sha. + """ + response = self.get_pullrequest_from_commit(repo, sha) + if not isinstance(response, list) or len(response) != 1: + # the response should return a single merged PR, return if multiple + if len(response) > 1: + return None + + if response[0]["state"] == "open": + metrics.incr( + "github_pr_comment.queue_comment_check.open_pr", + sample_rate=1.0, + ) + return None + + return response[0].get("merge_commit_sha") + def get_pullrequest_from_commit(self, repo: str, sha: str) -> Any: """ https://docs.github.com/en/rest/commits/commits#list-pull-requests-associated-with-a-commit diff --git a/src/sentry/tasks/commit_context.py b/src/sentry/tasks/commit_context.py index 51a883c22b6e99..4e789a03793587 100644 --- a/src/sentry/tasks/commit_context.py +++ b/src/sentry/tasks/commit_context.py @@ -48,6 +48,8 @@ PR_COMMENT_TASK_TTL = timedelta(minutes=5).total_seconds() PR_COMMENT_WINDOW = 14 # days +PR_COMMENT_SUPPORTED_PROVIDERS = {"integrations:github"} + logger = logging.getLogger(__name__) @@ -63,35 +65,23 @@ def queue_comment_task_if_needed( # client will raise an Exception if the request is not successful try: - response = installation.get_client().get_pullrequest_from_commit( - repo=repo.name, sha=commit.key - ) + client = installation.get_client() + merge_commit_sha = client.get_merge_commit_sha_from_commit(repo=repo.name, sha=commit.key) except Exception as e: sentry_sdk.capture_exception(e) return - if not isinstance(response, list) or len(response) != 1: - # the response should return a single PR, return if multiple - if len(response) > 1: - logger.info( - "github.pr_comment.queue_comment_check.commit_not_in_default_branch", - extra={ - "organization_id": commit.organization_id, - "repository_id": repo.id, - "commit_sha": commit.key, - }, - ) - return - - if response[0]["state"] == "open": - metrics.incr( - "github_pr_comment.queue_comment_check.open_pr", - sample_rate=1.0, + if merge_commit_sha is None: + logger.info( + "github.pr_comment.queue_comment_check.commit_not_in_default_branch", + extra={ + "organization_id": commit.organization_id, + "repository_id": repo.id, + "commit_sha": commit.key, + }, ) return - merge_commit_sha = response[0]["merge_commit_sha"] - pr_query = PullRequest.objects.filter( organization_id=commit.organization_id, repository_id=commit.repository_id, @@ -288,13 +278,13 @@ def process_commit_context( "github.pr_comment", extra={"organization_id": project.organization_id}, ) - repo = Repository.objects.filter(id=commit.repository_id) + repo = Repository.objects.filter(id=commit.repository_id).order_by("-date_added") group = Group.objects.get_from_cache(id=group_id) if ( group.level is not logging.INFO # Don't comment on info level issues and installation is not None and repo.exists() - and repo.get().provider == "integrations:github" + and repo.get().provider in PR_COMMENT_SUPPORTED_PROVIDERS ): queue_comment_task_if_needed(commit, group_owner, repo.get(), installation) else: diff --git a/tests/sentry/integrations/github/test_client.py b/tests/sentry/integrations/github/test_client.py index d017a893c0a3ec..b2cab2171a847a 100644 --- a/tests/sentry/integrations/github/test_client.py +++ b/tests/sentry/integrations/github/test_client.py @@ -332,6 +332,40 @@ def test_get_comment_reactions(self, get_jwt): del stored_reactions["url"] assert reactions == stored_reactions + @mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1") + @responses.activate + def test_get_merge_commit_sha_from_commit(self, get_jwt): + merge_commit_sha = "jkl123" + pull_requests = [{"merge_commit_sha": merge_commit_sha, "state": "closed"}] + commit_sha = "asdf" + responses.add( + responses.GET, + f"https://api.github.com/repos/{self.repo.name}/commits/{commit_sha}/pulls", + json=pull_requests, + ) + + sha = self.github_client.get_merge_commit_sha_from_commit( + repo=self.repo.name, sha=commit_sha + ) + assert sha == merge_commit_sha + + @mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1") + @responses.activate + def test_get_merge_commit_sha_from_commit_open_pr(self, get_jwt): + merge_commit_sha = "jkl123" + pull_requests = [{"merge_commit_sha": merge_commit_sha, "state": "open"}] + commit_sha = "asdf" + responses.add( + responses.GET, + f"https://api.github.com/repos/{self.repo.name}/commits/{commit_sha}/pulls", + json=pull_requests, + ) + + sha = self.github_client.get_merge_commit_sha_from_commit( + repo=self.repo.name, sha=commit_sha + ) + assert sha is None + @responses.activate def test_disable_email(self): with self.tasks(): From 748aef2125c204b6e1d547d6dcd4773c80127ca8 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Tue, 16 Jul 2024 10:30:42 -0700 Subject: [PATCH 2/4] small tweaks --- src/sentry/integrations/github/client.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/sentry/integrations/github/client.py b/src/sentry/integrations/github/client.py index 2f945b305ce525..9f9f2813197671 100644 --- a/src/sentry/integrations/github/client.py +++ b/src/sentry/integrations/github/client.py @@ -229,19 +229,19 @@ def get_merge_commit_sha_from_commit(self, repo: str, sha: str) -> str | None: Get the merge commit sha from a commit sha. """ response = self.get_pullrequest_from_commit(repo, sha) - if not isinstance(response, list) or len(response) != 1: + if not response or isinstance(response, list) or len(response) != 1: # the response should return a single merged PR, return if multiple - if len(response) > 1: - return None + return None - if response[0]["state"] == "open": + pull_request = response[0] + if pull_request["state"] == "open": metrics.incr( "github_pr_comment.queue_comment_check.open_pr", sample_rate=1.0, ) return None - return response[0].get("merge_commit_sha") + return pull_request.get("merge_commit_sha") def get_pullrequest_from_commit(self, repo: str, sha: str) -> Any: """ From a5d48aab48ad3dabc18068ef3e38658baddb68cc Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Tue, 16 Jul 2024 11:00:00 -0700 Subject: [PATCH 3/4] fix test --- src/sentry/integrations/github/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/github/client.py b/src/sentry/integrations/github/client.py index 9f9f2813197671..881102eb417fd1 100644 --- a/src/sentry/integrations/github/client.py +++ b/src/sentry/integrations/github/client.py @@ -229,7 +229,7 @@ def get_merge_commit_sha_from_commit(self, repo: str, sha: str) -> str | None: Get the merge commit sha from a commit sha. """ response = self.get_pullrequest_from_commit(repo, sha) - if not response or isinstance(response, list) or len(response) != 1: + if not response or (isinstance(response, list) and len(response) != 1): # the response should return a single merged PR, return if multiple return None From 43f79bd86113dfe93870c5e3270a137954d06177 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Fri, 19 Jul 2024 09:54:34 -0700 Subject: [PATCH 4/4] use idiom --- src/sentry/integrations/github/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/github/client.py b/src/sentry/integrations/github/client.py index 881102eb417fd1..c2f88a9eb03b9c 100644 --- a/src/sentry/integrations/github/client.py +++ b/src/sentry/integrations/github/client.py @@ -233,7 +233,7 @@ def get_merge_commit_sha_from_commit(self, repo: str, sha: str) -> str | None: # the response should return a single merged PR, return if multiple return None - pull_request = response[0] + (pull_request,) = response if pull_request["state"] == "open": metrics.incr( "github_pr_comment.queue_comment_check.open_pr",