From b7ff5d8eb62c2884d68968433a3ef11166e4117e Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 7 Apr 2026 16:25:53 +0200 Subject: [PATCH 1/5] feat(preprod): Add snapshot PR comment task and wiring (EME-999) Add Celery task that posts/updates GitHub PR comments for snapshot comparisons. Follows the same pattern as build distribution PR comments: locks CommitComparison rows, finds existing comment across PR commits, creates or updates via GitHub API, stores comment_id in extras["pr_comments"]["snapshots"]. Wire the task into all snapshot trigger points: - Comparison completion/failure in snapshots/tasks.py - Upload completion in snapshot endpoint - Approval via API and GitHub check run webhook - Rerun status checks endpoint --- .../api/endpoints/preprod_artifact_approve.py | 9 + .../preprod_artifact_rerun_status_checks.py | 4 + .../endpoints/preprod_artifact_snapshot.py | 7 + src/sentry/preprod/snapshots/tasks.py | 13 + .../preprod/vcs/pr_comments/snapshot_tasks.py | 256 +++++++++ .../preprod/vcs/webhooks/github_check_run.py | 7 + .../vcs/pr_comments/test_snapshot_tasks.py | 488 ++++++++++++++++++ 7 files changed, 784 insertions(+) create mode 100644 src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py create mode 100644 tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py diff --git a/src/sentry/preprod/api/endpoints/preprod_artifact_approve.py b/src/sentry/preprod/api/endpoints/preprod_artifact_approve.py index a4eabbb6a98b36..d1c3afebdc92f7 100644 --- a/src/sentry/preprod/api/endpoints/preprod_artifact_approve.py +++ b/src/sentry/preprod/api/endpoints/preprod_artifact_approve.py @@ -15,6 +15,7 @@ ) from sentry.models.organization import Organization from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.size.tasks import create_preprod_status_check_task from sentry.preprod.vcs.status_checks.snapshots.tasks import ( create_preprod_snapshot_status_check_task, @@ -91,4 +92,12 @@ def post(self, request: Request, organization: Organization, artifact_id: str) - caller="approval_endpoint", ) + if feature_type == PreprodComparisonApproval.FeatureType.SNAPSHOTS: + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": artifact.id, + "caller": "approval_endpoint", + }, + ) + return Response({"detail": "Approved"}, status=201) diff --git a/src/sentry/preprod/api/endpoints/preprod_artifact_rerun_status_checks.py b/src/sentry/preprod/api/endpoints/preprod_artifact_rerun_status_checks.py index 00b45938d39641..43569010c63fac 100644 --- a/src/sentry/preprod/api/endpoints/preprod_artifact_rerun_status_checks.py +++ b/src/sentry/preprod/api/endpoints/preprod_artifact_rerun_status_checks.py @@ -13,6 +13,7 @@ from sentry.preprod.analytics import PreprodArtifactApiRerunStatusChecksEvent from sentry.preprod.api.bases.preprod_artifact_endpoint import PreprodArtifactEndpoint from sentry.preprod.models import PreprodArtifact +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.size.tasks import create_preprod_status_check_task from sentry.preprod.vcs.status_checks.snapshots.tasks import ( create_preprod_snapshot_status_check_task, @@ -100,6 +101,9 @@ def post( create_preprod_snapshot_status_check_task.delay( preprod_artifact_id=head_artifact.id, caller="rerun_endpoint" ) + create_preprod_snapshot_pr_comment_task.delay( + preprod_artifact_id=head_artifact.id, caller="rerun_endpoint" + ) case _: continue except Exception: diff --git a/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py b/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py index 5bf1b1c86ac9e4..419d263adefd6e 100644 --- a/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py +++ b/src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py @@ -59,6 +59,7 @@ find_head_snapshot_artifacts_awaiting_base, ) from sentry.preprod.url_utils import get_preprod_artifact_url +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.snapshots.tasks import ( create_preprod_snapshot_status_check_task, ) @@ -579,6 +580,12 @@ def post(self, request: Request, project: Project) -> Response: "caller": "upload_completion", }, ) + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": artifact.id, + "caller": "upload_completion", + }, + ) if base_sha and base_repo_name: try: diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index d6968ebb46c5e7..c6f19bb2de7bc2 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -22,6 +22,7 @@ SnapshotManifest, ) from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.snapshots.tasks import ( create_preprod_snapshot_status_check_task, ) @@ -717,6 +718,12 @@ def _fetch_hash(h: str) -> None: "caller": "compare_completion", }, ) + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": head_artifact_id, + "caller": "compare_completion", + }, + ) logger.info( "Snapshot comparison complete", @@ -759,4 +766,10 @@ def _fetch_hash(h: str) -> None: "caller": "compare_failure", }, ) + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": head_artifact_id, + "caller": "compare_failure", + }, + ) raise diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py new file mode 100644 index 00000000000000..72af19b8a53ed6 --- /dev/null +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py @@ -0,0 +1,256 @@ +from __future__ import annotations + +import logging +from collections.abc import Sequence +from typing import Any + +from django.db import router, transaction +from taskbroker_client.retry import Retry + +from sentry import features +from sentry.models.commitcomparison import CommitComparison +from sentry.preprod.integration_utils import get_commit_context_client +from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval +from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.vcs.pr_comments.snapshot_templates import format_snapshot_pr_comment +from sentry.preprod.vcs.pr_comments.tasks import _get_error_type +from sentry.preprod.vcs.status_checks.snapshots.tasks import ( + FAIL_ON_ADDED_OPTION_KEY, + FAIL_ON_REMOVED_OPTION_KEY, + _build_changes_map, +) +from sentry.shared_integrations.exceptions import ApiError +from sentry.silo.base import SiloMode +from sentry.tasks.base import instrumented_task +from sentry.taskworker.namespaces import preprod_tasks + +logger = logging.getLogger(__name__) + +ENABLED_OPTION_KEY = "sentry:preprod_snapshot_pr_comments_enabled" +FEATURE_FLAG = "organizations:preprod-snapshot-pr-comments" + + +@instrumented_task( + name="sentry.preprod.tasks.create_preprod_snapshot_pr_comment", + namespace=preprod_tasks, + processing_deadline_duration=30, + silo_mode=SiloMode.CELL, + retry=Retry(times=5, delay=60 * 5), +) +def create_preprod_snapshot_pr_comment_task( + preprod_artifact_id: int, caller: str | None = None, **kwargs: Any +) -> None: + try: + artifact = PreprodArtifact.objects.select_related( + "mobile_app_info", + "commit_comparison", + "project", + "project__organization", + ).get(id=preprod_artifact_id) + except PreprodArtifact.DoesNotExist: + logger.exception( + "preprod.snapshot_pr_comments.create.artifact_not_found", + extra={"artifact_id": preprod_artifact_id, "caller": caller}, + ) + return + + if not artifact.commit_comparison: + logger.info( + "preprod.snapshot_pr_comments.create.no_commit_comparison", + extra={"artifact_id": artifact.id}, + ) + return + + commit_comparison = artifact.commit_comparison + if ( + not commit_comparison.pr_number + or not commit_comparison.head_repo_name + or not commit_comparison.provider + ): + logger.info( + "preprod.snapshot_pr_comments.create.no_pr_info", + extra={ + "artifact_id": artifact.id, + "pr_number": commit_comparison.pr_number, + "head_repo_name": commit_comparison.head_repo_name, + }, + ) + return + + if not artifact.project.get_option(ENABLED_OPTION_KEY, default=True): + logger.info( + "preprod.snapshot_pr_comments.create.project_disabled", + extra={"artifact_id": artifact.id, "project_id": artifact.project.id}, + ) + return + + organization = artifact.project.organization + if not features.has(FEATURE_FLAG, organization): + logger.info( + "preprod.snapshot_pr_comments.create.feature_disabled", + extra={"artifact_id": artifact.id, "organization_id": organization.id}, + ) + return + + client = get_commit_context_client( + organization, commit_comparison.head_repo_name, commit_comparison.provider + ) + if not client: + logger.info( + "preprod.snapshot_pr_comments.create.no_client", + extra={"artifact_id": artifact.id}, + ) + return + + api_error: Exception | None = None + + with transaction.atomic(router.db_for_write(CommitComparison)): + all_for_pr = list( + CommitComparison.objects.select_for_update() + .filter( + organization_id=commit_comparison.organization_id, + head_repo_name=commit_comparison.head_repo_name, + pr_number=commit_comparison.pr_number, + ) + .order_by("id") + ) + + try: + cc = next(c for c in all_for_pr if c.id == commit_comparison.id) + except StopIteration: + raise CommitComparison.DoesNotExist( + f"CommitComparison {commit_comparison.id} was deleted before lock acquisition" + ) + + # Gather snapshot data + all_artifacts = list(artifact.get_sibling_artifacts_for_commit()) + + artifact_ids = [a.id for a in all_artifacts] + snapshot_metrics_qs = PreprodSnapshotMetrics.objects.filter( + preprod_artifact_id__in=artifact_ids, + ) + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics] = { + m.preprod_artifact_id: m for m in snapshot_metrics_qs + } + + all_artifacts = [a for a in all_artifacts if a.id in snapshot_metrics_map] + if not all_artifacts: + return + + metrics_ids = [m.id for m in snapshot_metrics_map.values()] + comparisons_qs = PreprodSnapshotComparison.objects.filter( + head_snapshot_metrics_id__in=metrics_ids, + ) + comparisons_map: dict[int, PreprodSnapshotComparison] = { + c.head_snapshot_metrics_id: c for c in comparisons_qs + } + + approvals_map: dict[int, PreprodComparisonApproval] = {} + approval_qs = PreprodComparisonApproval.objects.filter( + preprod_artifact_id__in=artifact_ids, + preprod_feature_type=PreprodComparisonApproval.FeatureType.SNAPSHOTS, + approval_status=PreprodComparisonApproval.ApprovalStatus.APPROVED, + ) + for approval in approval_qs: + approvals_map[approval.preprod_artifact_id] = approval + + base_artifact_map = PreprodArtifact.get_base_artifacts_for_commit(all_artifacts) + + fail_on_added = artifact.project.get_option(FAIL_ON_ADDED_OPTION_KEY, default=False) + fail_on_removed = artifact.project.get_option(FAIL_ON_REMOVED_OPTION_KEY, default=True) + changes_map = _build_changes_map( + all_artifacts, + snapshot_metrics_map, + comparisons_map, + fail_on_added=fail_on_added, + fail_on_removed=fail_on_removed, + ) + + comment_body = format_snapshot_pr_comment( + all_artifacts, + snapshot_metrics_map, + comparisons_map, + base_artifact_map, + changes_map, + approvals_map=approvals_map, + ) + + existing_comment_id = _find_existing_comment_id(all_for_pr) + + try: + if existing_comment_id: + client.update_comment( + repo=cc.head_repo_name, + issue_id=str(cc.pr_number), + comment_id=str(existing_comment_id), + data={"body": comment_body}, + ) + comment_id = existing_comment_id + logger.info( + "preprod.snapshot_pr_comments.create.updated", + extra={"artifact_id": artifact.id, "comment_id": comment_id}, + ) + else: + resp = client.create_comment( + repo=cc.head_repo_name, + issue_id=str(cc.pr_number), + data={"body": comment_body}, + ) + comment_id = str(resp["id"]) + logger.info( + "preprod.snapshot_pr_comments.create.created", + extra={"artifact_id": artifact.id, "comment_id": comment_id}, + ) + except Exception as e: + extra: dict[str, Any] = { + "artifact_id": artifact.id, + "organization_id": organization.id, + "error_type": type(e).__name__, + } + if isinstance(e, ApiError): + extra["status_code"] = e.code + logger.exception("preprod.snapshot_pr_comments.create.failed", extra=extra) + _save_pr_comment_result(cc, success=False, error=e) + api_error = e + else: + _save_pr_comment_result(cc, success=True, comment_id=comment_id) + + if api_error is not None: + raise api_error + + +def _find_existing_comment_id( + comparisons: Sequence[CommitComparison], +) -> str | None: + for cc in comparisons: + extras = cc.extras or {} + comment_id = extras.get("pr_comments", {}).get("snapshots", {}).get("comment_id") + if comment_id: + return str(comment_id) + return None + + +def _save_pr_comment_result( + commit_comparison: CommitComparison, + success: bool, + comment_id: str | None = None, + error: Exception | None = None, +) -> None: + extras = commit_comparison.extras or {} + + # Preserve the existing comment_id on failure so retries use + # update_comment instead of creating a duplicate. + if not comment_id: + existing = extras.get("pr_comments", {}).get("snapshots", {}) + comment_id = existing.get("comment_id") + + result: dict[str, Any] = {"success": success} + if comment_id: + result["comment_id"] = comment_id + if not success: + result["error_type"] = _get_error_type(error) + + pr_comments = extras.setdefault("pr_comments", {}) + pr_comments["snapshots"] = result + commit_comparison.extras = extras + commit_comparison.save(update_fields=["extras"]) diff --git a/src/sentry/preprod/vcs/webhooks/github_check_run.py b/src/sentry/preprod/vcs/webhooks/github_check_run.py index 6f14af81b92746..f6314439fcfa84 100644 --- a/src/sentry/preprod/vcs/webhooks/github_check_run.py +++ b/src/sentry/preprod/vcs/webhooks/github_check_run.py @@ -16,6 +16,7 @@ from sentry.models.repository import Repository from sentry.preprod.analytics import PreprodStatusCheckApprovalCreatedEvent from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.size.tasks import ( APPROVE_SIZE_ACTION_IDENTIFIER, create_preprod_status_check_task, @@ -224,5 +225,11 @@ def handle_preprod_check_run_event( preprod_artifact_id=artifact.id, caller="github_approve_webhook", ) + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": artifact.id, + "caller": "github_approve_webhook", + }, + ) else: raise ValueError(f"Unknown identifier: {identifier}") diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py new file mode 100644 index 00000000000000..af6eb3c1a53730 --- /dev/null +++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py @@ -0,0 +1,488 @@ +from __future__ import annotations + +import uuid +from unittest.mock import Mock, patch + +import pytest + +from sentry.models.commitcomparison import CommitComparison +from sentry.models.repository import Repository +from sentry.preprod.models import PreprodArtifact +from sentry.preprod.snapshots.models import PreprodSnapshotMetrics +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task +from sentry.shared_integrations.exceptions import ApiError +from sentry.testutils.cases import TestCase +from sentry.testutils.silo import cell_silo_test + +_sentinel = object() + + +@cell_silo_test +class CreatePreprodSnapshotPrCommentTaskTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.organization = self.create_organization(owner=self.user) + self.team = self.create_team(organization=self.organization) + self.project = self.create_project( + teams=[self.team], organization=self.organization, name="test_project" + ) + self._feature = "organizations:preprod-snapshot-pr-comments" + + def _create_artifact_with_metrics( + self, + pr_number: int | None = 42, + provider: str = "github", + app_id: str = "com.example.app", + image_count: int = 10, + commit_comparison: CommitComparison | None = _sentinel, + with_commit_comparison: bool = True, + ) -> tuple[PreprodArtifact, PreprodSnapshotMetrics]: + if commit_comparison is _sentinel: + if with_commit_comparison: + unique = str(uuid.uuid4()).replace("-", "")[:8] + commit_comparison = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha=unique.ljust(40, "a"), + base_sha=unique.ljust(40, "b"), + provider=provider, + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=pr_number, + ) + else: + commit_comparison = None + + artifact = self.create_preprod_artifact( + project=self.project, + state=PreprodArtifact.ArtifactState.PROCESSED, + app_id=app_id, + commit_comparison=commit_comparison, + app_name="TestApp", + build_version="1.0.0", + build_number=1, + ) + metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=artifact, + image_count=image_count, + ) + + artifact = PreprodArtifact.objects.select_related( + "mobile_app_info", + "commit_comparison", + "project", + "project__organization", + ).get(id=artifact.id) + + return artifact, metrics + + def _create_repo(self) -> Repository: + return Repository.objects.create( + organization_id=self.organization.id, + name="owner/repo", + provider="integrations:github", + integration_id=123, + ) + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_creates_comment(self, mock_format, mock_get_client): + mock_client = Mock() + mock_client.create_comment.return_value = {"id": 99999} + mock_get_client.return_value = mock_client + mock_format.return_value = "## Sentry Snapshot Testing\n..." + + artifact, metrics = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.create_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + data={"body": "## Sentry Snapshot Testing\n..."}, + ) + + assert artifact.commit_comparison is not None + artifact.commit_comparison.refresh_from_db() + snapshots = artifact.commit_comparison.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is True + assert snapshots["comment_id"] == "99999" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_updates_existing_comment(self, mock_format, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + mock_format.return_value = "updated body" + + commit_comparison = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="a" * 40, + base_sha="b" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + extras={"pr_comments": {"snapshots": {"success": True, "comment_id": "existing_123"}}}, + ) + + artifact, metrics = self._create_artifact_with_metrics( + commit_comparison=commit_comparison, + ) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.update_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + comment_id="existing_123", + data={"body": "updated body"}, + ) + mock_client.create_comment.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_no_commit_comparison(self, mock_get_client): + artifact, metrics = self._create_artifact_with_metrics(with_commit_comparison=False) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_get_client.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_no_pr_number(self, mock_get_client): + artifact, metrics = self._create_artifact_with_metrics(pr_number=None) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_get_client.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_no_client(self, mock_get_client): + mock_get_client.return_value = None + artifact, metrics = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_get_client.assert_called_once() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_no_snapshot_metrics(self, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + + # Create artifact without snapshot metrics + unique = str(uuid.uuid4()).replace("-", "")[:8] + cc = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha=unique.ljust(40, "a"), + base_sha=unique.ljust(40, "b"), + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + artifact = self.create_preprod_artifact( + project=self.project, + state=PreprodArtifact.ArtifactState.PROCESSED, + app_id="com.example.app", + commit_comparison=cc, + ) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.create_comment.assert_not_called() + mock_client.update_comment.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_project_option_disabled(self, mock_get_client): + self.project.update_option("sentry:preprod_snapshot_pr_comments_enabled", False) + artifact, metrics = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_get_client.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_when_feature_flag_disabled(self, mock_get_client): + artifact, metrics = self._create_artifact_with_metrics() + + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_get_client.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + def test_skips_nonexistent_artifact(self, mock_get_client): + create_preprod_snapshot_pr_comment_task(99999) + + mock_get_client.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_handles_api_error(self, mock_format, mock_get_client): + mock_client = Mock() + mock_client.create_comment.side_effect = ApiError("rate limited", code=429) + mock_get_client.return_value = mock_client + mock_format.return_value = "body" + + artifact, metrics = self._create_artifact_with_metrics() + + with self.feature(self._feature): + with pytest.raises(ApiError): + create_preprod_snapshot_pr_comment_task(artifact.id) + + assert artifact.commit_comparison is not None + artifact.commit_comparison.refresh_from_db() + snapshots = artifact.commit_comparison.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is False + assert snapshots["error_type"] == "api_error" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_retry_after_update_failure_uses_update(self, mock_format, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + mock_format.return_value = "body" + + commit_comparison = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="e" * 40, + base_sha="f" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + extras={"pr_comments": {"snapshots": {"success": True, "comment_id": "orig_789"}}}, + ) + + artifact, metrics = self._create_artifact_with_metrics( + commit_comparison=commit_comparison, + ) + + # First call: update fails + mock_client.update_comment.side_effect = ApiError("server error", code=500) + + with self.feature(self._feature): + with pytest.raises(ApiError): + create_preprod_snapshot_pr_comment_task(artifact.id) + + commit_comparison.refresh_from_db() + snapshots = commit_comparison.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is False + assert snapshots["comment_id"] == "orig_789" + + # Second call (retry): should use update_comment + mock_client.reset_mock() + mock_client.update_comment.side_effect = None + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.update_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + comment_id="orig_789", + data={"body": "body"}, + ) + mock_client.create_comment.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_retry_after_create_failure_creates_again(self, mock_format, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + mock_format.return_value = "body" + + artifact, metrics = self._create_artifact_with_metrics() + + # First call: create fails + mock_client.create_comment.side_effect = ApiError("server error", code=500) + + with self.feature(self._feature): + with pytest.raises(ApiError): + create_preprod_snapshot_pr_comment_task(artifact.id) + + assert artifact.commit_comparison is not None + artifact.commit_comparison.refresh_from_db() + snapshots = artifact.commit_comparison.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is False + assert "comment_id" not in snapshots + + # Second call (retry): should create again + mock_client.reset_mock() + mock_client.create_comment.side_effect = None + mock_client.create_comment.return_value = {"id": 77777} + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.create_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + data={"body": "body"}, + ) + mock_client.update_comment.assert_not_called() + + artifact.commit_comparison.refresh_from_db() + snapshots = artifact.commit_comparison.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is True + assert snapshots["comment_id"] == "77777" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_reads_fresh_extras_via_select_for_update(self, mock_format, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + mock_format.return_value = "updated body" + + commit_comparison = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="c" * 40, + base_sha="d" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + + artifact, metrics = self._create_artifact_with_metrics( + commit_comparison=commit_comparison, + ) + + # Simulate a concurrent task writing a comment_id after artifact load + CommitComparison.objects.filter(id=commit_comparison.id).update( + extras={"pr_comments": {"snapshots": {"success": True, "comment_id": "concurrent_456"}}} + ) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.update_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + comment_id="concurrent_456", + data={"body": "updated body"}, + ) + mock_client.create_comment.assert_not_called() + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_updates_comment_from_previous_commit(self, mock_format, mock_get_client): + mock_client = Mock() + mock_get_client.return_value = mock_client + mock_format.return_value = "updated body" + + # Commit A already posted a comment + cc_a = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="a" * 40, + base_sha="b" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + extras={ + "pr_comments": {"snapshots": {"success": True, "comment_id": "prev_commit_123"}} + }, + ) + + # Commit B on the same PR — no comment_id yet + cc_b = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="c" * 40, + base_sha="d" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + + artifact, metrics = self._create_artifact_with_metrics(commit_comparison=cc_b) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.update_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + comment_id="prev_commit_123", + data={"body": "updated body"}, + ) + mock_client.create_comment.assert_not_called() + + # comment_id stored on cc_b as well + cc_b.refresh_from_db() + snapshots = cc_b.extras["pr_comments"]["snapshots"] + assert snapshots["comment_id"] == "prev_commit_123" + + # cc_a unchanged + cc_a.refresh_from_db() + assert cc_a.extras["pr_comments"]["snapshots"]["comment_id"] == "prev_commit_123" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + def test_creates_when_no_sibling_has_comment_id(self, mock_format, mock_get_client): + mock_client = Mock() + mock_client.create_comment.return_value = {"id": 55555} + mock_get_client.return_value = mock_client + mock_format.return_value = "new body" + + CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="a" * 40, + base_sha="b" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + + cc_b = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="c" * 40, + base_sha="d" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + + artifact, metrics = self._create_artifact_with_metrics(commit_comparison=cc_b) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_client.create_comment.assert_called_once_with( + repo="owner/repo", + issue_id="42", + data={"body": "new body"}, + ) + mock_client.update_comment.assert_not_called() + + cc_b.refresh_from_db() + snapshots = cc_b.extras["pr_comments"]["snapshots"] + assert snapshots["success"] is True + assert snapshots["comment_id"] == "55555" From 30f425aa293e75221b0d89011079c865741fd519 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Thu, 9 Apr 2026 09:20:11 +0200 Subject: [PATCH 2/5] fix(preprod): Fix mypy type error for sentinel default argument --- tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py index af6eb3c1a53730..757b19741505fa 100644 --- a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py +++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py @@ -34,7 +34,7 @@ def _create_artifact_with_metrics( provider: str = "github", app_id: str = "com.example.app", image_count: int = 10, - commit_comparison: CommitComparison | None = _sentinel, + commit_comparison: CommitComparison | None | object = _sentinel, with_commit_comparison: bool = True, ) -> tuple[PreprodArtifact, PreprodSnapshotMetrics]: if commit_comparison is _sentinel: From a8544569162d72d92274719e2432adcb8b193641 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Thu, 9 Apr 2026 09:44:35 +0200 Subject: [PATCH 3/5] ref(preprod): Extract shared PR comment save/find helpers Parameterize _find_existing_comment_id and _save_pr_comment_result by comment_type so both build_distribution and snapshots tasks use the same logic instead of maintaining near-identical copies. --- .../preprod/vcs/pr_comments/snapshot_tasks.py | 46 ++----------------- src/sentry/preprod/vcs/pr_comments/tasks.py | 25 ++++++++-- 2 files changed, 25 insertions(+), 46 deletions(-) diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py index 72af19b8a53ed6..f815c635fc337d 100644 --- a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py @@ -1,7 +1,6 @@ from __future__ import annotations import logging -from collections.abc import Sequence from typing import Any from django.db import router, transaction @@ -13,7 +12,7 @@ from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics from sentry.preprod.vcs.pr_comments.snapshot_templates import format_snapshot_pr_comment -from sentry.preprod.vcs.pr_comments.tasks import _get_error_type +from sentry.preprod.vcs.pr_comments.tasks import find_existing_comment_id, save_pr_comment_result from sentry.preprod.vcs.status_checks.snapshots.tasks import ( FAIL_ON_ADDED_OPTION_KEY, FAIL_ON_REMOVED_OPTION_KEY, @@ -175,7 +174,7 @@ def create_preprod_snapshot_pr_comment_task( approvals_map=approvals_map, ) - existing_comment_id = _find_existing_comment_id(all_for_pr) + existing_comment_id = find_existing_comment_id(all_for_pr, "snapshots") try: if existing_comment_id: @@ -210,47 +209,10 @@ def create_preprod_snapshot_pr_comment_task( if isinstance(e, ApiError): extra["status_code"] = e.code logger.exception("preprod.snapshot_pr_comments.create.failed", extra=extra) - _save_pr_comment_result(cc, success=False, error=e) + save_pr_comment_result(cc, "snapshots", success=False, error=e) api_error = e else: - _save_pr_comment_result(cc, success=True, comment_id=comment_id) + save_pr_comment_result(cc, "snapshots", success=True, comment_id=comment_id) if api_error is not None: raise api_error - - -def _find_existing_comment_id( - comparisons: Sequence[CommitComparison], -) -> str | None: - for cc in comparisons: - extras = cc.extras or {} - comment_id = extras.get("pr_comments", {}).get("snapshots", {}).get("comment_id") - if comment_id: - return str(comment_id) - return None - - -def _save_pr_comment_result( - commit_comparison: CommitComparison, - success: bool, - comment_id: str | None = None, - error: Exception | None = None, -) -> None: - extras = commit_comparison.extras or {} - - # Preserve the existing comment_id on failure so retries use - # update_comment instead of creating a duplicate. - if not comment_id: - existing = extras.get("pr_comments", {}).get("snapshots", {}) - comment_id = existing.get("comment_id") - - result: dict[str, Any] = {"success": success} - if comment_id: - result["comment_id"] = comment_id - if not success: - result["error_type"] = _get_error_type(error) - - pr_comments = extras.setdefault("pr_comments", {}) - pr_comments["snapshots"] = result - commit_comparison.extras = extras - commit_comparison.save(update_fields=["extras"]) diff --git a/src/sentry/preprod/vcs/pr_comments/tasks.py b/src/sentry/preprod/vcs/pr_comments/tasks.py index 4bd9b715e6d1cb..38811642f36d7b 100644 --- a/src/sentry/preprod/vcs/pr_comments/tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/tasks.py @@ -176,17 +176,34 @@ def create_preprod_pr_comment_task( def _find_existing_comment_id( comparisons: Sequence[CommitComparison], +) -> str | None: + return find_existing_comment_id(comparisons, "build_distribution") + + +def _save_pr_comment_result( + commit_comparison: CommitComparison, + success: bool, + comment_id: str | None = None, + error: Exception | None = None, +) -> None: + save_pr_comment_result(commit_comparison, "build_distribution", success, comment_id, error) + + +def find_existing_comment_id( + comparisons: Sequence[CommitComparison], + comment_type: str, ) -> str | None: for cc in comparisons: extras = cc.extras or {} - comment_id = extras.get("pr_comments", {}).get("build_distribution", {}).get("comment_id") + comment_id = extras.get("pr_comments", {}).get(comment_type, {}).get("comment_id") if comment_id: return str(comment_id) return None -def _save_pr_comment_result( +def save_pr_comment_result( commit_comparison: CommitComparison, + comment_type: str, success: bool, comment_id: str | None = None, error: Exception | None = None, @@ -201,7 +218,7 @@ def _save_pr_comment_result( # Preserve the existing comment_id on failure so retries use # update_comment instead of creating a duplicate. if not comment_id: - existing = extras.get("pr_comments", {}).get("build_distribution", {}) + existing = extras.get("pr_comments", {}).get(comment_type, {}) comment_id = existing.get("comment_id") result: dict[str, Any] = {"success": success} @@ -211,7 +228,7 @@ def _save_pr_comment_result( result["error_type"] = _get_error_type(error) pr_comments = extras.setdefault("pr_comments", {}) - pr_comments["build_distribution"] = result + pr_comments[comment_type] = result commit_comparison.extras = extras commit_comparison.save(update_fields=["extras"]) From 89cd881418d3083e8094d6676441412eb325a7d9 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Thu, 9 Apr 2026 16:55:37 +0200 Subject: [PATCH 4/5] feat(preprod): Add trigger script for snapshot PR comments Adds a manual trigger script for testing snapshot PR comments locally, mirroring the existing trigger_pr_comment and trigger_snapshot_status_check scripts. --- bin/preprod/trigger_snapshot_pr_comment | 396 ++++++++++++++++++++++++ 1 file changed, 396 insertions(+) create mode 100755 bin/preprod/trigger_snapshot_pr_comment diff --git a/bin/preprod/trigger_snapshot_pr_comment b/bin/preprod/trigger_snapshot_pr_comment new file mode 100755 index 00000000000000..ed4d7813dc93c5 --- /dev/null +++ b/bin/preprod/trigger_snapshot_pr_comment @@ -0,0 +1,396 @@ +#!/usr/bin/env python + +from sentry.runner import configure + +configure() + +import logging +import sys +from concurrent.futures import as_completed +from datetime import timedelta + +logger = logging.getLogger(__name__) + +from sentry.models.commitcomparison import CommitComparison +from sentry.models.project import Project +from sentry.preprod.models import ( + PreprodArtifact, + PreprodArtifactMobileAppInfo, + PreprodComparisonApproval, +) +from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task +from sentry.utils.concurrent import ContextPropagatingThreadPoolExecutor as ThreadPoolExecutor + +################# +# Configuration # +################# +ORG_SLUG = "sentry" +PROJECT_SLUG = "internal" +REPO_NAME = "EmergeTools/hackernews" +PR_NUMBER = 784 +COMMIT_SHA = "46fb48581ba0e8cdcaa01765a1123d996f5272d3" +BASE_COMMIT_SHA = "a79c91dacdbc32c0cc64e5251062a5d210089e64" + +# Snapshot build configurations +BUILDS = [ + { + "app_id": "com.emerge.hackernews.android", + "artifact_type": PreprodArtifact.ArtifactType.AAB, + "app_name": "HackerNews Android", + "build_version": "1.0.3", + "build_number": 42, + "state": "compared_with_changes", + "create_base": True, + "images_changed": 3, + "images_added": 1, + "images_removed": 0, + "images_unchanged": 20, + "image_count": 24, + }, + { + "app_id": "com.emerge.hackernews.ios", + "artifact_type": PreprodArtifact.ArtifactType.XCARCHIVE, + "app_name": "HackerNews iOS", + "build_version": "1.0.3", + "build_number": 15, + "state": "compared_no_changes", + "create_base": True, + "images_changed": 0, + "images_added": 0, + "images_removed": 0, + "images_unchanged": 15, + "image_count": 15, + }, +] + +# Available states: +# - "no_metrics": Artifact has no snapshot metrics yet (processing) +# - "metrics_no_comparison": Artifact has metrics but no comparison yet +# - "comparison_pending": Comparison created but still pending +# - "comparison_processing": Comparison in progress +# - "comparison_failed": Comparison failed +# - "compared_no_changes": Comparison complete, all images match +# - "compared_with_changes": Comparison complete, some images differ + +# Pass --concurrent to trigger all builds in parallel (tests lock serialization) +CONCURRENT = "--concurrent" in sys.argv + + +def cleanup_existing_data(project, commit_comparison): + """Remove existing test artifacts for idempotent reruns.""" + logger.info("\nCleaning up existing artifacts for commit %s...", COMMIT_SHA[:8]) + + base_commit_comparisons = CommitComparison.objects.filter( + head_sha=BASE_COMMIT_SHA, + organization_id=project.organization_id, + ) + + all_commit_comparisons = [commit_comparison] + list(base_commit_comparisons) + existing_artifacts = PreprodArtifact.objects.filter( + commit_comparison__in=all_commit_comparisons, + project__organization_id=project.organization_id, + ) + + if existing_artifacts.exists(): + artifact_ids = list(existing_artifacts.values_list("id", flat=True)) + metrics = PreprodSnapshotMetrics.objects.filter(preprod_artifact_id__in=artifact_ids) + metrics_ids = list(metrics.values_list("id", flat=True)) + + comparison_count = PreprodSnapshotComparison.objects.filter( + head_snapshot_metrics_id__in=metrics_ids + ).delete()[0] + metrics_count = metrics.delete()[0] + approval_count = PreprodComparisonApproval.objects.filter( + preprod_artifact_id__in=artifact_ids + ).delete()[0] + artifact_count = existing_artifacts.delete()[0] + + if base_commit_comparisons.exists(): + base_count = base_commit_comparisons.delete()[0] + logger.info( + " Deleted %s artifacts, %s metrics, %s comparisons, %s approvals, %s base commits", + artifact_count, + metrics_count, + comparison_count, + approval_count, + base_count, + ) + else: + logger.info( + " Deleted %s artifacts, %s metrics, %s comparisons, %s approvals", + artifact_count, + metrics_count, + comparison_count, + approval_count, + ) + else: + logger.info(" No existing artifacts found (clean slate)") + + # Clear any previous snapshot comment_id so we get a fresh create + extras = commit_comparison.extras or {} + pr_comments = extras.get("pr_comments", {}) + if "snapshots" in pr_comments: + del pr_comments["snapshots"] + commit_comparison.extras = extras + commit_comparison.save(update_fields=["extras"]) + logger.info(" Cleared previous snapshot comment_id from extras") + + +def create_artifact_and_metrics(project, build_config, commit_comparison): + """Create a preprod artifact with snapshot metrics.""" + state = build_config["state"] + + artifact = PreprodArtifact.objects.create( + project=project, + artifact_type=build_config["artifact_type"], + app_id=build_config["app_id"], + commit_comparison=commit_comparison, + state=PreprodArtifact.ArtifactState.PROCESSED, + ) + PreprodArtifactMobileAppInfo.objects.create( + preprod_artifact=artifact, + app_name=build_config["app_name"], + build_version=build_config["build_version"], + build_number=build_config["build_number"], + ) + + artifact.date_added = artifact.date_added - timedelta(minutes=5) + artifact.save(update_fields=["date_added"]) + + logger.info(" Created artifact: %s (ID: %s)", build_config["app_id"], artifact.id) + + if state == "no_metrics": + logger.info(" No metrics created (simulating upload in progress)") + return artifact, None + + metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=artifact, + image_count=build_config.get("image_count", 10), + ) + logger.info(" Created snapshot metrics: %s (%s images)", metrics.id, metrics.image_count) + + return artifact, metrics + + +def create_base_data(project, build_config, head_commit_comparison): + """Create base artifact and metrics for comparison.""" + base_commit_comparison, _ = CommitComparison.objects.get_or_create( + head_repo_name=head_commit_comparison.head_repo_name, + head_sha=BASE_COMMIT_SHA, + provider=head_commit_comparison.provider, + organization_id=head_commit_comparison.organization_id, + ) + + if not head_commit_comparison.base_sha: + head_commit_comparison.base_sha = BASE_COMMIT_SHA + head_commit_comparison.save(update_fields=["base_sha"]) + + base_artifact = PreprodArtifact.objects.create( + project=project, + artifact_type=build_config["artifact_type"], + app_id=build_config["app_id"], + commit_comparison=base_commit_comparison, + state=PreprodArtifact.ArtifactState.PROCESSED, + ) + PreprodArtifactMobileAppInfo.objects.create( + preprod_artifact=base_artifact, + app_name=build_config["app_name"], + build_version=build_config["build_version"], + build_number=build_config["build_number"] - 1, + ) + + base_metrics = PreprodSnapshotMetrics.objects.create( + preprod_artifact=base_artifact, + image_count=build_config.get("image_count", 10), + ) + + logger.info( + " Created base artifact: %s (ID: %s) with metrics", + build_config["app_id"], + base_artifact.id, + ) + + return base_metrics + + +def create_comparison(build_config, head_metrics, base_metrics): + """Create a snapshot comparison based on the configured state.""" + state = build_config["state"] + + state_map = { + "comparison_pending": PreprodSnapshotComparison.State.PENDING, + "comparison_processing": PreprodSnapshotComparison.State.PROCESSING, + "comparison_failed": PreprodSnapshotComparison.State.FAILED, + "compared_no_changes": PreprodSnapshotComparison.State.SUCCESS, + "compared_with_changes": PreprodSnapshotComparison.State.SUCCESS, + } + + comparison_state = state_map.get(state) + if comparison_state is None: + return None + + comparison = PreprodSnapshotComparison.objects.create( + head_snapshot_metrics=head_metrics, + base_snapshot_metrics=base_metrics, + state=comparison_state, + images_changed=build_config.get("images_changed", 0), + images_added=build_config.get("images_added", 0), + images_removed=build_config.get("images_removed", 0), + images_unchanged=build_config.get("images_unchanged", 0), + ) + + status_label = { + PreprodSnapshotComparison.State.PENDING: "Pending", + PreprodSnapshotComparison.State.PROCESSING: "Processing", + PreprodSnapshotComparison.State.FAILED: "Failed", + PreprodSnapshotComparison.State.SUCCESS: "Success", + }[comparison_state] + + logger.info(" Created comparison: %s", status_label) + if comparison_state == PreprodSnapshotComparison.State.SUCCESS: + logger.info( + " Changed: %s, Added: %s, Removed: %s, Unchanged: %s", + comparison.images_changed, + comparison.images_added, + comparison.images_removed, + comparison.images_unchanged, + ) + + return comparison + + +def setup_builds(project, build_configs, commit_comparison): + """Create all artifacts, metrics, base data, and comparisons.""" + created_artifacts = [] + + for build_config in build_configs: + logger.info("\nSetting up %s...", build_config["app_id"]) + + artifact, head_metrics = create_artifact_and_metrics( + project, build_config, commit_comparison + ) + created_artifacts.append(artifact) + + if ( + head_metrics + and build_config.get("create_base") + and build_config["state"] + not in ( + "no_metrics", + "metrics_no_comparison", + ) + ): + base_metrics = create_base_data(project, build_config, commit_comparison) + create_comparison(build_config, head_metrics, base_metrics) + + return created_artifacts + + +def run_task(artifact_id): + """Run the snapshot PR comment task for an artifact.""" + create_preprod_snapshot_pr_comment_task(preprod_artifact_id=artifact_id) + + +def main(): + try: + project = Project.objects.get(slug=PROJECT_SLUG, organization__slug=ORG_SLUG) + logger.info("Found project: %s (ID: %s)", project.name, project.id) + + commit_comparison, created = CommitComparison.objects.get_or_create( + head_repo_name=REPO_NAME, + head_sha=COMMIT_SHA, + provider="github", + organization_id=project.organization.id, + defaults={ + "head_ref": "main", + "base_ref": "main", + "pr_number": PR_NUMBER, + }, + ) + if not created and not commit_comparison.pr_number: + commit_comparison.pr_number = PR_NUMBER + commit_comparison.save(update_fields=["pr_number"]) + + logger.info( + "%s commit comparison: %s (pr_number=%s)", + "Created" if created else "Found", + commit_comparison.id, + commit_comparison.pr_number, + ) + + cleanup_existing_data(project, commit_comparison) + + logger.info("\nRunning snapshot PR comment test with %s build(s):", len(BUILDS)) + for i, config in enumerate(BUILDS, 1): + logger.info( + " BUILD_%s: %s (state: %s, base: %s)", + i, + config["app_id"], + config["state"], + config.get("create_base", False), + ) + + created_artifacts = setup_builds(project, BUILDS, commit_comparison) + + if CONCURRENT: + logger.info( + "\nTriggering %s tasks concurrently (testing lock serialization)...", + len(created_artifacts), + ) + + def run_build_task(artifact): + from django import db + + db.connections.close_all() + run_task(artifact.id) + return artifact.id + + with ThreadPoolExecutor(max_workers=len(created_artifacts)) as pool: + futures = {pool.submit(run_build_task, a): a.id for a in created_artifacts} + for future in as_completed(futures): + aid = futures[future] + try: + future.result() + logger.info(" Task for artifact %s completed", aid) + except Exception: + logger.exception(" Task for artifact %s failed", aid) + else: + logger.info( + "\nCalling create_preprod_snapshot_pr_comment_task with artifact %s...", + created_artifacts[0].id, + ) + run_task(created_artifacts[0].id) + + logger.info("Done!") + + # Show results + commit_comparison.refresh_from_db() + cc_extras = commit_comparison.extras or {} + snapshots = cc_extras.get("pr_comments", {}).get("snapshots", {}) + comment_id = snapshots.get("comment_id") + + if snapshots.get("success") and comment_id: + logger.info( + "\nComment posted! Check: https://github.com/%s/pull/%s", + REPO_NAME, + PR_NUMBER, + ) + logger.info(" Comment ID: %s", comment_id) + elif not snapshots.get("success") and snapshots: + logger.info("\nComment FAILED. Error: %s", snapshots.get("error_type")) + else: + logger.info("\nComment was NOT posted. Check logs above for early-return reasons.") + + except Project.DoesNotExist: + logger.exception("Project not found: %s/%s", ORG_SLUG, PROJECT_SLUG) + for p in Project.objects.select_related("organization").all()[:10]: + logger.info(" - %s/%s", p.organization.slug, p.slug) + sys.exit(1) + except Exception: + logger.exception("Error running trigger_snapshot_pr_comment") + sys.exit(1) + + +if __name__ == "__main__": + main() From 6a9ab12a6d66ae66e8729c4bd071cb196f5d4e6e Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Fri, 10 Apr 2026 10:40:30 +0200 Subject: [PATCH 5/5] ref(preprod): Inline wrapper functions and extract build_changes_map Remove single-line wrapper functions _find_existing_comment_id and _save_pr_comment_result, inlining the calls with the hardcoded "build_distribution" argument directly at the call sites. Rename _build_changes_map to build_changes_map and move it (along with its helper _comparison_has_changes) from the status_checks snapshots tasks module to sentry.preprod.snapshots.utils, since it is now shared across multiple production modules. --- src/sentry/preprod/snapshots/utils.py | 39 ++++++++++++++++++- .../preprod/vcs/pr_comments/snapshot_tasks.py | 4 +- src/sentry/preprod/vcs/pr_comments/tasks.py | 21 ++-------- .../vcs/status_checks/snapshots/tasks.py | 37 +----------------- .../vcs/status_checks/snapshots/test_tasks.py | 14 +++---- 5 files changed, 52 insertions(+), 63 deletions(-) diff --git a/src/sentry/preprod/snapshots/utils.py b/src/sentry/preprod/snapshots/utils.py index 707e714b841902..c3668ba1115100 100644 --- a/src/sentry/preprod/snapshots/utils.py +++ b/src/sentry/preprod/snapshots/utils.py @@ -1,7 +1,10 @@ from __future__ import annotations from sentry.preprod.models import PreprodArtifact, PreprodBuildConfiguration -from sentry.preprod.snapshots.models import PreprodSnapshotComparison +from sentry.preprod.snapshots.models import ( + PreprodSnapshotComparison, + PreprodSnapshotMetrics, +) def find_base_snapshot_artifact( @@ -59,3 +62,37 @@ def find_head_snapshot_artifacts_awaiting_base( .select_related("preprodsnapshotmetrics") .order_by("-date_added") ) + + +def _comparison_has_changes( + comparison: PreprodSnapshotComparison, + fail_on_added: bool = False, + fail_on_removed: bool = True, +) -> bool: + return ( + comparison.images_changed > 0 + or comparison.images_renamed > 0 + or (fail_on_added and comparison.images_added > 0) + or (fail_on_removed and comparison.images_removed > 0) + ) + + +def build_changes_map( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], + comparisons_map: dict[int, PreprodSnapshotComparison], + fail_on_added: bool = False, + fail_on_removed: bool = True, +) -> dict[int, bool]: + changes_map: dict[int, bool] = {} + for artifact in artifacts: + metrics = snapshot_metrics_map.get(artifact.id) + if not metrics: + continue + comparison = comparisons_map.get(metrics.id) + if not comparison or comparison.state != PreprodSnapshotComparison.State.SUCCESS: + continue + changes_map[artifact.id] = _comparison_has_changes( + comparison, fail_on_added, fail_on_removed + ) + return changes_map diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py index f815c635fc337d..9e505367e58f5d 100644 --- a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py @@ -11,12 +11,12 @@ from sentry.preprod.integration_utils import get_commit_context_client from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.snapshots.utils import build_changes_map from sentry.preprod.vcs.pr_comments.snapshot_templates import format_snapshot_pr_comment from sentry.preprod.vcs.pr_comments.tasks import find_existing_comment_id, save_pr_comment_result from sentry.preprod.vcs.status_checks.snapshots.tasks import ( FAIL_ON_ADDED_OPTION_KEY, FAIL_ON_REMOVED_OPTION_KEY, - _build_changes_map, ) from sentry.shared_integrations.exceptions import ApiError from sentry.silo.base import SiloMode @@ -157,7 +157,7 @@ def create_preprod_snapshot_pr_comment_task( fail_on_added = artifact.project.get_option(FAIL_ON_ADDED_OPTION_KEY, default=False) fail_on_removed = artifact.project.get_option(FAIL_ON_REMOVED_OPTION_KEY, default=True) - changes_map = _build_changes_map( + changes_map = build_changes_map( all_artifacts, snapshot_metrics_map, comparisons_map, diff --git a/src/sentry/preprod/vcs/pr_comments/tasks.py b/src/sentry/preprod/vcs/pr_comments/tasks.py index 38811642f36d7b..140a3a1331cd83 100644 --- a/src/sentry/preprod/vcs/pr_comments/tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/tasks.py @@ -129,7 +129,7 @@ def create_preprod_pr_comment_task( if not installable_siblings: return - existing_comment_id = _find_existing_comment_id(all_for_pr) + existing_comment_id = find_existing_comment_id(all_for_pr, "build_distribution") comment_body = format_pr_comment(installable_siblings, project=artifact.project) try: @@ -165,30 +165,15 @@ def create_preprod_pr_comment_task( if isinstance(e, ApiError): extra["status_code"] = e.code logger.exception("preprod.pr_comments.create.failed", extra=extra) - _save_pr_comment_result(cc, success=False, error=e) + save_pr_comment_result(cc, "build_distribution", success=False, error=e) api_error = e else: - _save_pr_comment_result(cc, success=True, comment_id=comment_id) + save_pr_comment_result(cc, "build_distribution", success=True, comment_id=comment_id) if api_error is not None: raise api_error -def _find_existing_comment_id( - comparisons: Sequence[CommitComparison], -) -> str | None: - return find_existing_comment_id(comparisons, "build_distribution") - - -def _save_pr_comment_result( - commit_comparison: CommitComparison, - success: bool, - comment_id: str | None = None, - error: Exception | None = None, -) -> None: - save_pr_comment_result(commit_comparison, "build_distribution", success, comment_id, error) - - def find_existing_comment_id( comparisons: Sequence[CommitComparison], comment_type: str, diff --git a/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py b/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py index 30accd444b3aaa..671e2adc763ba8 100644 --- a/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py @@ -12,6 +12,7 @@ PreprodComparisonApproval, ) from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.snapshots.utils import build_changes_map from sentry.preprod.url_utils import get_preprod_artifact_url from sentry.preprod.vcs.status_checks.size.tasks import ( GITHUB_STATUS_CHECK_STATUS_MAPPING, @@ -147,7 +148,7 @@ def create_preprod_snapshot_status_check_task( is_solo = not base_artifact_map if not is_solo: - changes_map = _build_changes_map( + changes_map = build_changes_map( all_artifacts, snapshot_metrics_map, comparisons_map, @@ -315,40 +316,6 @@ def create_preprod_snapshot_status_check_task( ) -def _comparison_has_changes( - comparison: PreprodSnapshotComparison, - fail_on_added: bool = False, - fail_on_removed: bool = True, -) -> bool: - return ( - comparison.images_changed > 0 - or comparison.images_renamed > 0 - or (fail_on_added and comparison.images_added > 0) - or (fail_on_removed and comparison.images_removed > 0) - ) - - -def _build_changes_map( - artifacts: list[PreprodArtifact], - snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], - comparisons_map: dict[int, PreprodSnapshotComparison], - fail_on_added: bool = False, - fail_on_removed: bool = True, -) -> dict[int, bool]: - changes_map: dict[int, bool] = {} - for artifact in artifacts: - metrics = snapshot_metrics_map.get(artifact.id) - if not metrics: - continue - comparison = comparisons_map.get(metrics.id) - if not comparison or comparison.state != PreprodSnapshotComparison.State.SUCCESS: - continue - changes_map[artifact.id] = _comparison_has_changes( - comparison, fail_on_added, fail_on_removed - ) - return changes_map - - def _compute_snapshot_status( artifacts: list[PreprodArtifact], snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], diff --git a/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py b/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py index 5827fdf36e6752..ec3bd4f8705452 100644 --- a/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py +++ b/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py @@ -2,8 +2,8 @@ from sentry.integrations.source_code_management.status_check import StatusCheckStatus from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics +from sentry.preprod.snapshots.utils import build_changes_map from sentry.preprod.vcs.status_checks.snapshots.tasks import ( - _build_changes_map, _compute_snapshot_status, ) from sentry.testutils.cases import TestCase @@ -58,7 +58,7 @@ def _status_with_changes_map(self, artifact, metrics, approvals=None, **flags): artifacts = [artifact] metrics_map = {artifact.id: metrics} comparisons_map = {metrics.id: _get_comparison(metrics)} - changes_map = _build_changes_map(artifacts, metrics_map, comparisons_map, **flags) + changes_map = build_changes_map(artifacts, metrics_map, comparisons_map, **flags) return _compute_snapshot_status( artifacts, metrics_map, comparisons_map, approvals or {}, changes_map ) @@ -102,14 +102,14 @@ def test_no_changes_succeeds(self): class BuildChangesMapTest(SnapshotTasksTestBase): def test_added_ignored_when_flag_off(self): artifact, metrics, _ = self._make_artifact_with_comparison(images_added=5) - changes_map = _build_changes_map( + changes_map = build_changes_map( [artifact], {artifact.id: metrics}, {metrics.id: _get_comparison(metrics)} ) assert not any(changes_map.values()) def test_added_detected_when_flag_on(self): artifact, metrics, _ = self._make_artifact_with_comparison(images_added=5) - changes_map = _build_changes_map( + changes_map = build_changes_map( [artifact], {artifact.id: metrics}, {metrics.id: _get_comparison(metrics)}, @@ -119,7 +119,7 @@ def test_added_detected_when_flag_on(self): def test_removed_ignored_when_flag_off(self): artifact, metrics, _ = self._make_artifact_with_comparison(images_removed=3) - changes_map = _build_changes_map( + changes_map = build_changes_map( [artifact], {artifact.id: metrics}, {metrics.id: _get_comparison(metrics)}, @@ -129,14 +129,14 @@ def test_removed_ignored_when_flag_off(self): def test_removed_detected_by_default(self): artifact, metrics, _ = self._make_artifact_with_comparison(images_removed=3) - changes_map = _build_changes_map( + changes_map = build_changes_map( [artifact], {artifact.id: metrics}, {metrics.id: _get_comparison(metrics)} ) assert changes_map[artifact.id] is True def test_changed_always_detected(self): artifact, metrics, _ = self._make_artifact_with_comparison(images_changed=2) - changes_map = _build_changes_map( + changes_map = build_changes_map( [artifact], {artifact.id: metrics}, {metrics.id: _get_comparison(metrics)} ) assert changes_map[artifact.id] is True