Simplify get_installation_name a bit (#751)

helpers/github_installation.py

@@ -1,21 +1,21 @@
 import logging
 
-from sqlalchemy.orm import Session
-
 from database.models.core import (
     GITHUB_APP_INSTALLATION_DEFAULT_NAME,
     Owner,
     OwnerInstallationNameToUseForTask,
 )
-from helpers.cache import cache
 
 log = logging.getLogger(__file__)
 
 
-@cache.cache_function(ttl=86400)  # 1 day
-def get_installation_name_for_owner_for_task(
-    dbsession: Session, task_name: str, owner: Owner
-) -> str:
+def get_installation_name_for_owner_for_task(task_name: str, owner: Owner) -> str:
+    if owner.service not in ["github", "github_enterprise"]:
+        # The `installation` concept only exists in GitHub.
+        # We still return a default here, primarily to satisfy types.
+        return GITHUB_APP_INSTALLATION_DEFAULT_NAME
+
+    dbsession = owner.get_db_session()
     config_for_owner = (
         dbsession.query(OwnerInstallationNameToUseForTask)
         .filter(

helpers/tests/unit/test_github_installation.py

@@ -9,7 +9,7 @@
 
 
 def test_get_installation_name_for_owner_for_task(dbsession: Session):
-    owner = OwnerFactory()
+    owner = OwnerFactory(service="github")
     other_owner = OwnerFactory()
     task_name = "app.tasks.notify.Notify"
     installation_task_config = OwnerInstallationNameToUseForTask(
@@ -21,14 +21,13 @@ def test_get_installation_name_for_owner_for_task(dbsession: Session):
     dbsession.add_all([owner, installation_task_config])
     dbsession.flush()
     assert (
-        get_installation_name_for_owner_for_task(dbsession, task_name, owner)
-        == "my_installation"
+        get_installation_name_for_owner_for_task(task_name, owner) == "my_installation"
     )
     assert (
-        get_installation_name_for_owner_for_task(dbsession, task_name, other_owner)
+        get_installation_name_for_owner_for_task(task_name, other_owner)
         == GITHUB_APP_INSTALLATION_DEFAULT_NAME
     )
     assert (
-        get_installation_name_for_owner_for_task(dbsession, "other_task", owner)
+        get_installation_name_for_owner_for_task("other_task", owner)
         == GITHUB_APP_INSTALLATION_DEFAULT_NAME
     )

services/repository.py

@@ -45,7 +45,7 @@
 def get_repo_provider_service_for_specific_commit(
     commit: Commit,
     fallback_installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME,
-) -> torngit.base.TorngitBaseAdapter:
+) -> TorngitBaseAdapter:
     """Gets a Torngit adapter (potentially) using a specific GitHub app as the authentication source.
     If the commit doesn't have a particular app assigned to it, return regular `get_repo_provider_service` choice
 
@@ -84,13 +84,13 @@ def get_repo_provider_service_for_specific_commit(
         on_token_refresh=None,
         **data,
     )
-    return _get_repo_provider_service_instance(repository.service, **adapter_params)
+    return _get_repo_provider_service_instance(repository.service, adapter_params)
 
 
 @sentry_sdk.trace
 def get_repo_provider_service(
     repository: Repository,
-    installation_name_to_use: Optional[str] = GITHUB_APP_INSTALLATION_DEFAULT_NAME,
+    installation_name_to_use: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME,
 ) -> TorngitBaseAdapter:
     adapter_auth_info = get_adapter_auth_information(
         repository.owner,
@@ -121,10 +121,10 @@ def get_repo_provider_service(
         on_token_refresh=get_token_refresh_callback(adapter_auth_info["token_owner"]),
         **data,
     )
-    return _get_repo_provider_service_instance(repository.service, **adapter_params)
+    return _get_repo_provider_service_instance(repository.service, adapter_params)
 
 
-def _get_repo_provider_service_instance(service: str, **adapter_params):
+def _get_repo_provider_service_instance(service: str, adapter_params: dict):
     _timeouts = [
         get_config("setup", "http", "timeouts", "connect", default=30),
         get_config("setup", "http", "timeouts", "receive", default=60),

services/tests/test_repository_service.py

@@ -1178,10 +1178,12 @@ def test_get_repo_provider_service_for_specific_commit(
         )
         mock_get_instance.assert_called_with(
             "github",
-            **data,
-            token="the app token",
-            token_type_mapping=None,
-            on_token_refresh=None,
+            dict(
+                **data,
+                token="the app token",
+                token_type_mapping=None,
+                on_token_refresh=None,
+            ),
         )
 
 

tasks/bundle_analysis_notify.py

@@ -103,7 +103,7 @@ def process_impl_within_lock(
             }
 
         installation_name_to_use = get_installation_name_for_owner_for_task(
-            db_session, self.name, commit.repository.owner
+            self.name, commit.repository.owner
         )
         notifier = BundleAnalysisNotifyService(
             commit, commit_yaml, gh_app_installation_name=installation_name_to_use

tasks/commit_update.py

@@ -2,10 +2,7 @@
 
 from asgiref.sync import async_to_sync
 from shared.celery_config import commit_update_task_name
-from shared.torngit.exceptions import (
-    TorngitClientError,
-    TorngitRepoNotFoundError,
-)
+from shared.torngit.exceptions import TorngitClientError, TorngitRepoNotFoundError
 
 from app import celery_app
 from database.models import Commit
@@ -39,7 +36,7 @@ def run_impl(
         was_updated = False
         try:
             installation_name_to_use = get_installation_name_for_owner_for_task(
-                db_session, self.name, repository.owner
+                self.name, repository.owner
             )
             repository_service = get_repo_provider_service(
                 repository, installation_name_to_use=installation_name_to_use

tasks/compute_comparison.py

@@ -52,7 +52,7 @@ def run_impl(
         log.info("Computing comparison", extra=log_extra)
         current_yaml = get_repo_yaml(repo)
         installation_name_to_use = get_installation_name_for_owner_for_task(
-            db_session, self.name, repo.owner
+            self.name, repo.owner
         )
 
         comparison_proxy = self.get_comparison_proxy(

tasks/notify.py

@@ -197,11 +197,9 @@ def run_impl_within_lock(
             }
 
         try:
-            installation_name_to_use = None
-            if commit.repository.owner.service in ["github", "github_enterprise"]:
-                installation_name_to_use = get_installation_name_for_owner_for_task(
-                    db_session, self.name, commit.repository.owner
-                )
+            installation_name_to_use = get_installation_name_for_owner_for_task(
+                self.name, commit.repository.owner
+            )
             repository_service = get_repo_provider_service(
                 commit.repository, installation_name_to_use=installation_name_to_use
             )

tasks/preprocess_upload.py

@@ -91,7 +91,7 @@ def process_impl_within_lock(
         )
         assert commit, "Commit not found in database."
         installation_name_to_use = get_installation_name_for_owner_for_task(
-            db_session, self.name, commit.repository.owner
+            self.name, commit.repository.owner
         )
         repository_service = self.get_repo_service(commit, installation_name_to_use)
         if repository_service is None:

tasks/save_report_results.py

@@ -34,7 +34,7 @@ def run_impl(
 
         try:
             installation_name_to_use = get_installation_name_for_owner_for_task(
-                db_session, self.name, commit.repository.owner
+                self.name, commit.repository.owner
             )
             repository_service = get_repo_provider_service(
                 commit.repository, installation_name_to_use=installation_name_to_use

tasks/sync_pull.py

@@ -21,9 +21,7 @@
 from helpers.exceptions import RepositoryWithoutValidBotError
 from helpers.github_installation import get_installation_name_for_owner_for_task
 from helpers.metrics import metrics
-from rollouts import (
-    SYNC_PULL_USE_MERGE_COMMIT_SHA,
-)
+from rollouts import SYNC_PULL_USE_MERGE_COMMIT_SHA
 from services.comparison.changes import get_changes
 from services.redis import get_redis_connection
 from services.report import Report, ReportService
@@ -113,7 +111,7 @@ def run_impl_within_lock(
         assert repository
         try:
             installation_name_to_use = get_installation_name_for_owner_for_task(
-                db_session, self.name, repository.owner
+                self.name, repository.owner
             )
             repository_service = get_repo_provider_service(
                 repository, installation_name_to_use=installation_name_to_use

tasks/sync_repo_languages.py

@@ -55,7 +55,7 @@ def run_impl(
                 )
                 log.info("Syncing repository languages", extra=log_extra)
                 installation_name_to_use = get_installation_name_for_owner_for_task(
-                    db_session, self.name, repository.owner
+                    self.name, repository.owner
                 )
                 repository_service = get_repo_provider_service(
                     repository, installation_name_to_use=installation_name_to_use

tasks/upload.py

@@ -423,7 +423,7 @@ def run_impl_within_lock(
         was_updated, was_setup = False, False
         try:
             installation_name_to_use = get_installation_name_for_owner_for_task(
-                db_session, self.name, repository.owner
+                self.name, repository.owner
             )
             repository_service = get_repo_provider_service(
                 repository, installation_name_to_use=installation_name_to_use

tasks/upload_processor.py

@@ -456,7 +456,7 @@ def save_report_results(
         commitid = commit.commitid
         try:
             installation_name_to_use = get_installation_name_for_owner_for_task(
-                db_session, self.name, repository.owner
+                self.name, repository.owner
             )
             repository_service = get_repo_provider_service(
                 repository, installation_name_to_use=installation_name_to_use