feat: add task gh_app_name override for more tasks (#436)

* feat: add task gh_app_name override for more tasks

This adds ability for more tasks to use the existing configuration options and
provide overrides for the GitHubAppInstallation name to be used for a given user.

Some of these usages have to be propagated through the task, as is the case with
ComputeComparison, for example.

:warning: These are not all instances, but most of them. Still working on others.

* feat: Override gh_app selected by ReportService

This is actually only relevant when we are creating a new report,
with carryforward. So should affect mostly `Upload` and `PreProcessUpload` tasks.

In any case I've added to some tasks that make intense use of reports.

helpers/github_installation.py

@@ -1,3 +1,4 @@
+import logging
 from typing import Optional
 
 from sqlalchemy.orm import Session
@@ -9,6 +10,8 @@
 )
 from helpers.cache import cache
 
+log = logging.getLogger(__file__)
+
 
 @cache.cache_function(ttl=86400) # 1 day
 def get_installation_name_for_owner_for_task(
@@ -23,5 +26,9 @@ def get_installation_name_for_owner_for_task(
  .first()
  )
  if config_for_owner:
+ log.info(
+ "Owner has dedicated app for this task",
+ extra=dict(this_task=task_name, ownerid=owner.ownerid),
+ )
  return config_for_owner.installation_name
  return GITHUB_APP_INSTALLATION_DEFAULT_NAME

services/bundle_analysis.py

@@ -23,6 +23,7 @@
 
 from database.enums import ReportType
 from database.models import Commit, CommitReport, Repository, Upload, UploadError
+from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
 from services.archive import ArchiveService
 from services.report import BaseReportService
 from services.repository import (
@@ -275,9 +276,17 @@ class BundleRows:
 
 
 class Notifier:
- def __init__(self, commit: Commit, current_yaml: UserYaml):
+ def __init__(
+ self,
+ commit: Commit,
+ current_yaml: UserYaml,
+ gh_app_installation_name: str | None = None,
+ ):
  self.commit = commit
  self.current_yaml = current_yaml
+ self.gh_app_installation_name = (
+ gh_app_installation_name or GITHUB_APP_INSTALLATION_DEFAULT_NAME
+ )
 
  @cached_property
  def repository(self) -> Repository:
@@ -289,7 +298,10 @@ def commit_report(self) -> Optional[CommitReport]:
 
  @cached_property
  def repository_service(self) -> TorngitBaseAdapter:
- return get_repo_provider_service(self.commit.repository)
+ return get_repo_provider_service(
+ self.commit.repository,
+ installation_name_to_use=self.gh_app_installation_name,
+ )
 
  @cached_property
  def bundle_analysis_loader(self):

services/comparison/__init__.py

@@ -25,15 +25,15 @@
 
 
 @dataclass
-class NotificationContext(object):
+class ComparisonContext(object):
  """Extra information not necessarily related to coverage that may affect notifications"""
 
- all_tests_passed: bool
+ all_tests_passed: bool | None = None
  test_results_error: TestResultsProcessingError | None = None
+ gh_app_installation_name: str | None = None
 
 
 class ComparisonProxy(object):
-
  """The idea of this class is to produce a wrapper around Comparison with functionalities that
  are useful to the notifications context.
 
@@ -52,7 +52,7 @@ class ComparisonProxy(object):
  """
 
  def __init__(
- self, comparison: Comparison, context: Optional[NotificationContext] = None
+ self, comparison: Comparison, context: Optional[ComparisonContext] = None
  ):
  self.comparison = comparison
  self._repository_service = None
@@ -68,7 +68,7 @@ def __init__(
  self._behind_by_lock = asyncio.Lock()
  self._archive_service = None
  self._overlays = {}
- self.context = context
+ self.context = context or ComparisonContext()
 
  def get_archive_service(self):
  if self._archive_service is None:
@@ -87,7 +87,8 @@ def get_filtered_comparison(self, flags, path_patterns):
  def repository_service(self):
  if self._repository_service is None:
  self._repository_service = get_repo_provider_service(
- self.comparison.head.commit.repository
+ self.comparison.head.commit.repository,
+ installation_name_to_use=self.context.gh_app_installation_name,
  )
  return self._repository_service
 
@@ -291,11 +292,13 @@ def __init__(self, real_comparison: ComparisonProxy, *, flags, path_patterns):
  self._changes = None
  self.project_coverage_base = FullCommit(
  commit=real_comparison.project_coverage_base.commit,
- report=real_comparison.project_coverage_base.report.filter(
- flags=flags, paths=path_patterns
- )
- if self.has_project_coverage_base_report()
- else None,
+ report=(
+ real_comparison.project_coverage_base.report.filter(
+ flags=flags, paths=path_patterns
+ )
+ if self.has_project_coverage_base_report()
+ else None
+ ),
  )
  self.head = FullCommit(
  commit=real_comparison.head.commit,

services/comparison/overlays/__init__.py

@@ -8,5 +8,8 @@ class OverlayType(Enum):
 
 
 def get_overlay(overlay_type: OverlayType, comparison, **kwargs):
+ """
+ @param comparison: ComparisonProxy (not imported due to circular imports)
+ """
  if overlay_type == OverlayType.line_execution_count:
  return CriticalPathOverlay.init_from_comparison(comparison, **kwargs)

services/comparison/overlays/critical_path.py

@@ -83,7 +83,8 @@ async def _get_critical_files_from_yaml(self, filenames_to_search: Sequence[str]
  current_yaml = self._comparison.comparison.current_yaml
  if current_yaml is None:
  repo = self._comparison.head.commit.repository
- repo_provider = get_repo_provider_service(repo)
+ gh_app_installation_name = self._comparison.context.gh_app_installation_name
+ repo_provider = get_repo_provider_service(repo, gh_app_installation_name)
  current_yaml = await get_current_yaml(
  self._comparison.head.commit, repo_provider
  )

services/notification/notifiers/tests/integration/test_comment.py

@@ -5,7 +5,7 @@
 
 from database.enums import TestResultsProcessingError
 from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory
-from services.comparison import ComparisonProxy, NotificationContext
+from services.comparison import ComparisonContext, ComparisonProxy
 from services.comparison.types import Comparison, EnrichedPull, FullCommit
 from services.decoration import Decoration
 from services.notification.notifiers.comment import CommentNotifier
@@ -338,7 +338,7 @@ def sample_comparison_for_limited_upload(
 class TestCommentNotifierIntegration(object):
  @pytest.mark.asyncio
  async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration):
- sample_comparison.context = NotificationContext(
+ sample_comparison.context = ComparisonContext(
  all_tests_passed=True, test_results_error=None
  )
  mock_configuration._params["setup"] = {
@@ -412,7 +412,7 @@ async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration):
  async def test_notify_test_results_error(
  self, sample_comparison, codecov_vcr, mock_configuration
  ):
- sample_comparison.context = NotificationContext(
+ sample_comparison.context = ComparisonContext(
  all_tests_passed=False,
  test_results_error=TestResultsProcessingError.NO_SUCCESS,
  )

services/notification/notifiers/tests/unit/test_comment.py

@@ -20,7 +20,7 @@
 from database.tests.factories import RepositoryFactory
 from database.tests.factories.core import CommitFactory, OwnerFactory, PullFactory
 from services.billing import BillingPlan
-from services.comparison import ComparisonProxy, NotificationContext
+from services.comparison import ComparisonContext, ComparisonProxy
 from services.comparison.overlays.critical_path import CriticalPathOverlay
 from services.comparison.types import Comparison, FullCommit
 from services.decoration import Decoration
@@ -4028,7 +4028,7 @@ async def test_new_header_section_writer_with_behind_by(
  async def test_new_header_section_writer_test_results_setup(
  self, mocker, sample_comparison
  ):
- sample_comparison.context = NotificationContext(all_tests_passed=True)
+ sample_comparison.context = ComparisonContext(all_tests_passed=True)
  writer = NewHeaderSectionWriter(
  mocker.MagicMock(),
  mocker.MagicMock(),
@@ -4059,7 +4059,7 @@ async def test_new_header_section_writer_test_results_setup(
  async def test_new_header_section_writer_test_results_error(
  self, mocker, sample_comparison
  ):
- sample_comparison.context = NotificationContext(
+ sample_comparison.context = ComparisonContext(
  all_tests_passed=False,
  test_results_error=TestResultsProcessingError.NO_SUCCESS,
  )
@@ -4120,7 +4120,7 @@ async def test_new_header_section_writer_no_project_coverage(
  async def test_new_header_section_writer_no_project_coverage_test_results_setup(
  self, mocker, sample_comparison
  ):
- sample_comparison.context = NotificationContext(all_tests_passed=True)
+ sample_comparison.context = ComparisonContext(all_tests_passed=True)
  writer = NewHeaderSectionWriter(
  mocker.MagicMock(),
  mocker.MagicMock(),
@@ -4150,7 +4150,7 @@ async def test_new_header_section_writer_no_project_coverage_test_results_setup(
  async def test_new_header_section_writer_no_project_coverage_test_results_error(
  self, mocker, sample_comparison
  ):
- sample_comparison.context = NotificationContext(
+ sample_comparison.context = ComparisonContext(
  all_tests_passed=False,
  test_results_error=TestResultsProcessingError.NO_SUCCESS,
  )
@@ -4898,7 +4898,7 @@ async def test_build_message_team_plan_customer_all_lines_covered(
  sample_comparison_coverage_carriedforward,
  ):
  mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
- sample_comparison_coverage_carriedforward.context = NotificationContext(
+ sample_comparison_coverage_carriedforward.context = ComparisonContext(
  all_tests_passed=True
  )
  comparison = sample_comparison_coverage_carriedforward
@@ -4939,7 +4939,7 @@ async def test_build_message_team_plan_customer_all_lines_covered_test_results_e
  sample_comparison_coverage_carriedforward,
  ):
  mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
- sample_comparison_coverage_carriedforward.context = NotificationContext(
+ sample_comparison_coverage_carriedforward.context = ComparisonContext(
  all_tests_passed=False,
  test_results_error=TestResultsProcessingError.NO_SUCCESS,
  )
@@ -4981,7 +4981,7 @@ async def test_build_message_team_plan_customer_all_lines_covered(
  sample_comparison_coverage_carriedforward,
  ):
  mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
- sample_comparison_coverage_carriedforward.context = NotificationContext(
+ sample_comparison_coverage_carriedforward.context = ComparisonContext(
  all_tests_passed=False,
  test_results_error=None,
  )

services/report/__init__.py

@@ -188,9 +188,12 @@ def create_report_upload(
 class ReportService(BaseReportService):
  metrics_prefix = "services.report"
 
- def __init__(self, current_yaml: UserYaml):
+ def __init__(
+ self, current_yaml: UserYaml, gh_app_installation_name: str | None = None
+ ):
  super().__init__(current_yaml)
  self.flag_dict = None
+ self.gh_app_installation_name = gh_app_installation_name
 
  def has_initialized_report(self, commit: Commit) -> bool:
  """Says whether a commit has already initialized its report or not
@@ -693,14 +696,15 @@ async def _possibly_shift_carryforward_report(
  ):
  try:
  provider_service = get_repo_provider_service(
- repository=head_commit.repository
+ repository=head_commit.repository,
+ installation_name_to_use=self.gh_app_installation_name,
  )
  diff = (
  await provider_service.get_compare(
  base=base_commit.commitid, head=head_commit.commitid
  )
  )["diff"]
- # Volitile function, alters carryforward_report
+ # Volatile function, alters carryforward_report
  carryforward_report.shift_lines_by_diff(diff)
  except (RepositoryWithoutValidBotError, OwnerWithoutValidBotError) as exp:
  log.error(
@@ -898,7 +902,7 @@ def build_report_from_raw_content(
  ) -> ProcessingResult:
  """
  Processes an upload on top of an existing report `master` and returns
- a result, which could be succesful or not
+ a result, which could be successful or not
 
  Note that this function does not modify the `upload` object, as this should
  be done by a separate function
@@ -1189,7 +1193,11 @@ async def save_parallel_report_to_archive(
 
  # Attempt to calculate diff of report (which uses commit info from the git provider), but it it fails to do so, it just moves on without such diff
  try:
- repository_service = get_repo_provider_service(repository, commit)
+ repository_service = get_repo_provider_service(
+ repository,
+ commit,
+ installation_name_to_use=self.gh_app_installation_name,
+ )
  report.apply_diff(await repository_service.get_commit_diff(commitid))
  except TorngitError:
  # When this happens, we have that commit.totals["diff"] is not available.

tasks/bundle_analysis_notify.py

@@ -8,6 +8,7 @@
 from app import celery_app
 from database.enums import ReportType
 from database.models import Commit
+from helpers.github_installation import get_installation_name_for_owner_for_task
 from services.bundle_analysis import Notifier
 from services.lock_manager import LockManager, LockRetry, LockType
 from tasks.base import BaseCodecovTask
@@ -101,7 +102,12 @@ def process_impl_within_lock(
 
  success = None
  if notify:
- notifier = Notifier(commit, commit_yaml)
+ installation_name_to_use = get_installation_name_for_owner_for_task(
+ db_session, self.name, commit.repository.owner
+ )
+ notifier = Notifier(
+ commit, commit_yaml, gh_app_installation_name=installation_name_to_use
+ )
  success = async_to_sync(notifier.notify)()
 
  log.info(

tasks/commit_update.py

@@ -11,6 +11,7 @@
 from app import celery_app
 from database.models import Commit
 from helpers.exceptions import RepositoryWithoutValidBotError
+from helpers.github_installation import get_installation_name_for_owner_for_task
 from services.repository import (
  get_repo_provider_service,
  possibly_update_commit_from_provider_info,
@@ -38,7 +39,12 @@ def run_impl(
  repository_service = None
  was_updated = False
  try:
- repository_service = get_repo_provider_service(repository, commit)
+ installation_name_to_use = get_installation_name_for_owner_for_task(
+ db_session, self.name, repository.owner
+ )
+ repository_service = get_repo_provider_service(
+ repository, commit, installation_name_to_use=installation_name_to_use
+ )
  was_updated = async_to_sync(possibly_update_commit_from_provider_info)(
  commit, repository_service
  )