feat: pin down commits to apps (#482)

conftest.py

@@ -210,6 +210,7 @@ def mock_repo_provider(mocker):
     m = mocker.patch("services.repository._get_repo_provider_service_instance")
     provider_instance = mocker.MagicMock(
         GithubHandler,
+        data={},
         get_commit_diff=mock.AsyncMock(return_value={}),
         get_distance_in_commits=mock.AsyncMock(
             return_value={"behind_by": 0, "behind_by_commit": None}

services/github.py

@@ -69,7 +69,8 @@ def get_github_app_for_commit(commit: Commit) -> str | None:
         return None
     redis = get_redis_connection()
     try:
-        return redis.get(COMMIT_GHAPP_KEY_NAME(commit.id))
+        value = redis.get(COMMIT_GHAPP_KEY_NAME(commit.id))
+        return value if value is None else value.decode()
     except RedisError:
         log.exception(
             "Failed to get app for commit", extra=dict(commit=commit.commitid)

services/notification/__init__.py

@@ -6,17 +6,16 @@
 """
 
 import asyncio
-import dataclasses
 import logging
-from typing import Iterator, List
+from typing import Iterator, List, TypedDict
 
 from celery.exceptions import CeleryError, SoftTimeLimitExceeded
 from shared.config import get_config
 from shared.helpers.yaml import default_if_true
 from shared.plan.constants import TEAM_PLAN_REPRESENTATIONS
 from shared.yaml import UserYaml
 
-from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME, Owner
+from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME, Owner, Repository
 from helpers.metrics import metrics
 from services.comparison import ComparisonProxy
 from services.decoration import Decoration
@@ -45,10 +44,16 @@
 log = logging.getLogger(__name__)
 
 
+class IndividualResult(TypedDict):
+    notifier: str
+    title: str
+    result: NotificationResult | None
+
+
 class NotificationService(object):
     def __init__(
         self,
-        repository,
+        repository: Repository,
         current_yaml: UserYaml,
         decoration_type=Decoration.standard,
         gh_installation_name_to_use: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME,
@@ -238,9 +243,11 @@ async def notify(self, comparison: ComparisonProxy) -> List[NotificationResult]:
             f"Notifying with decoration type {self.decoration_type}",
             extra=dict(
                 head_commit=comparison.head.commit.commitid,
-                base_commit=comparison.project_coverage_base.commit.commitid
-                if comparison.project_coverage_base.commit is not None
-                else "NO_BASE",
+                base_commit=(
+                    comparison.project_coverage_base.commit.commitid
+                    if comparison.project_coverage_base.commit is not None
+                    else "NO_BASE"
+                ),
                 repoid=comparison.head.commit.repoid,
             ),
         )
@@ -268,60 +275,51 @@ async def notify_individual_notifier(
             "Attempting individual notification",
             extra=dict(
                 commit=commit.commitid,
-                base_commit=base_commit.commitid
-                if base_commit is not None
-                else "NO_BASE",
+                base_commit=(
+                    base_commit.commitid if base_commit is not None else "NO_BASE"
+                ),
                 repoid=commit.repoid,
                 notifier=notifier.name,
                 notifier_title=notifier.title,
             ),
         )
+        # individual_result.result is updated in case of success
+        individual_result = IndividualResult(
+            notifier=notifier.name, title=notifier.title, result=None
+        )
         try:
             with metrics.timer(
                 f"worker.services.notifications.notifiers.{notifier.name}"
             ) as notify_timer:
                 res = await notifier.notify(comparison)
-            individual_result = {
-                "notifier": notifier.name,
-                "title": notifier.title,
-                "result": dataclasses.asdict(res),
-            }
+            individual_result["result"] = res
+
             notifier.store_results(comparison, res)
             log.info(
                 "Individual notification done",
                 extra=dict(
                     timing_ms=notify_timer.ms,
                     individual_result=individual_result,
                     commit=commit.commitid,
-                    base_commit=base_commit.commitid
-                    if base_commit is not None
-                    else "NO_BASE",
+                    base_commit=(
+                        base_commit.commitid if base_commit is not None else "NO_BASE"
+                    ),
                     repoid=commit.repoid,
                 ),
             )
             return individual_result
         except (CeleryError, SoftTimeLimitExceeded):
-            individual_result = {
-                "notifier": notifier.name,
-                "title": notifier.title,
-                "result": None,
-            }
             raise
         except asyncio.TimeoutError:
-            individual_result = {
-                "notifier": notifier.name,
-                "title": notifier.title,
-                "result": None,
-            }
             log.warning(
                 "Individual notifier timed out",
                 extra=dict(
                     repoid=commit.repoid,
                     commit=commit.commitid,
                     individual_result=individual_result,
-                    base_commit=base_commit.commitid
-                    if base_commit is not None
-                    else "NO_BASE",
+                    base_commit=(
+                        base_commit.commitid if base_commit is not None else "NO_BASE"
+                    ),
                 ),
             )
             return individual_result
@@ -333,36 +331,27 @@ async def notify_individual_notifier(
                 ),
                 exc_info=True,
             )
-            individual_result = {
-                "notifier": notifier.name,
-                "title": notifier.title,
-                "result": None,
-            }
             raise
         except Exception:
-            individual_result = {
-                "notifier": notifier.name,
-                "title": notifier.title,
-                "result": None,
-            }
             log.exception(
                 "Individual notifier failed",
                 extra=dict(
                     repoid=commit.repoid,
                     commit=commit.commitid,
                     individual_result=individual_result,
-                    base_commit=base_commit.commitid
-                    if base_commit is not None
-                    else "NO_BASE",
+                    base_commit=(
+                        base_commit.commitid if base_commit is not None else "NO_BASE"
+                    ),
                 ),
             )
             return individual_result
         finally:
-            if not individual_result["result"] or individual_result["result"].get(
-                "notification_attempted"
+            if (
+                individual_result["result"] is None
+                or individual_result["result"].notification_attempted
             ):
                 # only running if there is no result (indicating some exception)
                 # or there was an actual attempt
                 create_or_update_commit_notification_from_notification_result(
-                    comparison.pull, notifier, individual_result["result"]
+                    comparison, notifier, individual_result["result"]
                 )

services/notification/commit_notifications.py

@@ -1,22 +1,30 @@
 import logging
 
+from sqlalchemy.orm.session import Session
+
 from database.enums import NotificationState
 from database.models import CommitNotification, Pull
 from helpers.metrics import metrics
-from services.notification.notifiers.base import AbstractBaseNotifier
+from services.comparison import ComparisonProxy
+from services.notification.notifiers.base import (
+    AbstractBaseNotifier,
+    NotificationResult,
+)
 
 log = logging.getLogger(__name__)
 
 
-def _get_notification_state_from_result(result_dict) -> NotificationState:
+def _get_notification_state_from_result(
+    notification_result: NotificationResult | None,
+) -> NotificationState:
     """
-    Take notification result_dict from notification service and convert to
+    Take notification result from notification service and convert to
     the proper NotificationState enum
     """
-    if result_dict is None:
+    if notification_result is None:
         return NotificationState.error
 
-    successful = result_dict.get("notification_successful")
+    successful = notification_result.notification_successful
 
     if successful:
         return NotificationState.success
@@ -26,18 +34,35 @@ def _get_notification_state_from_result(result_dict) -> NotificationState:
 
 @metrics.timer("internal.services.notification.store_notification_result")
 def create_or_update_commit_notification_from_notification_result(
-    pull: Pull, notifier: AbstractBaseNotifier, result_dict
+    comparison: ComparisonProxy,
+    notifier: AbstractBaseNotifier,
+    notification_result: NotificationResult | None,
 ) -> CommitNotification:
-    if not pull:
+    """Saves a CommitNotification entry in the database.
+    We save an entry in the following scenarios:
+        - We save all notification attempts for commits that are part of a PullRequest
+        - We save _successful_ notification attempt _with_ a github app
+    """
+    pull: Pull | None = comparison.pull
+    not_pull = pull is None
+    not_head_commit = comparison.head is None or comparison.head.commit is None
+    not_github_app_info = (
+        notification_result is None or notification_result.github_app_used is None
+    )
+    failed = (
+        notification_result is None
+        or notification_result.notification_successful == False
+    )
+    if not_pull and (not_head_commit or not_github_app_info or failed):
         return
 
-    db_session = pull.get_db_session()
-
-    commit = pull.get_head_commit()
+    commit = pull.get_head_commit() if pull else comparison.head.commit
     if not commit:
         log.warning("Head commit not found for pull", extra=dict(pull=pull))
         return
 
+    db_session: Session = commit.get_db_session()
+
     commit_notification = (
         db_session.query(CommitNotification)
         .filter(
@@ -47,13 +72,17 @@ def create_or_update_commit_notification_from_notification_result(
         .first()
     )
 
-    notification_state = _get_notification_state_from_result(result_dict)
+    notification_state = _get_notification_state_from_result(notification_result)
+    github_app_used = (
+        notification_result.github_app_used if notification_result else None
+    )
 
     if not commit_notification:
         commit_notification = CommitNotification(
             commit_id=commit.id_,
             notification_type=notifier.notification_type,
             decoration_type=notifier.decoration_type,
+            gh_app_id=github_app_used,
             state=notification_state,
         )
         db_session.add(commit_notification)

services/notification/notifiers/base.py

@@ -17,6 +17,7 @@ class NotificationResult(object):
     explanation: str = None
     data_sent: Mapping[str, Any] = None
     data_received: Mapping[str, Any] = None
+    github_app_used: int | None = None
 
     def merge(self, other: "NotificationResult") -> "NotificationResult":
         ans = NotificationResult()
@@ -60,10 +61,10 @@ def __init__(
         :param title: The project name for this notification, if applicable. For more info see https://docs.codecov.io/docs/commit-status#splitting-up-projects-example
         :param notifier_yaml_settings: Contains the codecov yaml fields, if any, for this particular notification.
             example: status -> patch -> custom_project_name -> <whatever is here is in notifier_yaml_settings for custom_project_name's status patch notifier>
-        :param notifier_site_settings -> Contains the codecov yaml fields under the "notify" header
-        :param current_yaml -> The complete codecov yaml used for this notification.
-        :param decoration_type -> Indicates whether the user needs to upgrade their account before they can see the notification
-        :param gh_installation_name_to_use -> [GitHub exclusive] propagates choice of the installation_name in case of gh multi apps
+        :param notifier_site_settings: Contains the codecov yaml fields under the "notify" header
+        :param current_yaml: The complete codecov yaml used for this notification.
+        :param decoration_type: Indicates whether the user needs to upgrade their account before they can see the notification
+        :param gh_installation_name_to_use: [GitHub exclusive] propagates choice of the installation_name in case of gh multi apps
         """
         self.repository = repository
         self.title = title
@@ -90,7 +91,7 @@ def store_results(self, comparison: Comparison, result: NotificationResult):
 
         Args:
             comparison (Comparison): The comparison with which this notify ran
-            result (NotificationResult): The results of the notificaiton
+            result (NotificationResult): The results of the notification
         """
         raise NotImplementedError()
 

services/notification/notifiers/checks/base.py

@@ -7,7 +7,6 @@
 from helpers.metrics import metrics
 from services.notification.notifiers.base import Comparison, NotificationResult
 from services.notification.notifiers.status.base import StatusNotifier
-from services.repository import get_repo_provider_service
 from services.urls import (
     append_tracking_params_to_urls,
     get_commit_url,
@@ -344,19 +343,10 @@ def create_annotations(self, comparison, diff):
             annotations.append(annotation)
         return annotations
 
-    @property
-    def repository_service(self):
-        if not self._repository_service:
-            self._repository_service = get_repo_provider_service(
-                self.repository, installation_name_to_use=self.gh_installation_name
-            )
-        return self._repository_service
-
     async def send_notification(self, comparison: Comparison, payload):
         title = self.get_status_external_name()
-        repository_service = self.repository_service
         head = comparison.head.commit
-        head_report = comparison.head.report
+        repository_service = self.repository_service(head)
         state = payload["state"]
         state = "success" if self.notifier_yaml_settings.get("informational") else state
 
@@ -438,4 +428,5 @@ async def send_notification(self, comparison: Comparison, payload):
             notification_successful=True,
             explanation=None,
             data_sent=payload,
+            github_app_used=self.get_github_app_used(),
         )