From 9f5f7b914fead30e65f9a621e6ad804515f73628 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 30 Sep 2024 16:09:40 +0200 Subject: [PATCH] Simplifies `get_installation_name` a bit This concept is only relevant for github, so move an appropriate check inside. Also removes the superfluous `dbsession` parameter, and removes the caching which is quite the overkill for this very simple query. --- helpers/github_installation.py | 14 +++++++------- helpers/tests/unit/test_github_installation.py | 9 ++++----- services/repository.py | 14 +++++++++----- services/tests/test_repository_service.py | 10 ++++++---- tasks/bundle_analysis_notify.py | 2 +- tasks/commit_update.py | 7 ++----- tasks/compute_comparison.py | 2 +- tasks/notify.py | 8 +++----- tasks/preprocess_upload.py | 2 +- tasks/save_report_results.py | 2 +- tasks/sync_pull.py | 6 ++---- tasks/sync_repo_languages.py | 2 +- tasks/upload.py | 2 +- tasks/upload_processor.py | 2 +- 14 files changed, 40 insertions(+), 42 deletions(-) diff --git a/helpers/github_installation.py b/helpers/github_installation.py index ebe0f7b0d..202ede12d 100644 --- a/helpers/github_installation.py +++ b/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( diff --git a/helpers/tests/unit/test_github_installation.py b/helpers/tests/unit/test_github_installation.py index 8ff5f0896..7b9170096 100644 --- a/helpers/tests/unit/test_github_installation.py +++ b/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 ) diff --git a/services/repository.py b/services/repository.py index 33a0f6760..fb9536cac 100644 --- a/services/repository.py +++ b/services/repository.py @@ -31,6 +31,7 @@ from database.enums import CommitErrorTypes from database.models import Commit, Owner, Pull, Repository from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME +from helpers.cache import cache from helpers.save_commit_error import save_commit_error from helpers.token_refresh import get_token_refresh_callback from services.github import get_github_app_for_commit @@ -45,7 +46,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 +85,16 @@ 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 +@cache.cache_function( + ttl=5 * 60 # this TTL is quite conservative, it could potentially be higher +) 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 +125,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), diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index b56c4b321..97fafca91 100644 --- a/services/tests/test_repository_service.py +++ b/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, + ), ) diff --git a/tasks/bundle_analysis_notify.py b/tasks/bundle_analysis_notify.py index 086030b5b..d57f5cd0b 100644 --- a/tasks/bundle_analysis_notify.py +++ b/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 diff --git a/tasks/commit_update.py b/tasks/commit_update.py index 284273b63..fb30ed9b4 100644 --- a/tasks/commit_update.py +++ b/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 diff --git a/tasks/compute_comparison.py b/tasks/compute_comparison.py index 461e3642b..69ae83be9 100644 --- a/tasks/compute_comparison.py +++ b/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( diff --git a/tasks/notify.py b/tasks/notify.py index 8c9b64ca5..159a1c4f4 100644 --- a/tasks/notify.py +++ b/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 ) diff --git a/tasks/preprocess_upload.py b/tasks/preprocess_upload.py index cc38522f5..3417f125c 100644 --- a/tasks/preprocess_upload.py +++ b/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: diff --git a/tasks/save_report_results.py b/tasks/save_report_results.py index a4498e2f2..b66148ce9 100644 --- a/tasks/save_report_results.py +++ b/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 diff --git a/tasks/sync_pull.py b/tasks/sync_pull.py index 23fdd64a9..a8ffeeb10 100644 --- a/tasks/sync_pull.py +++ b/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 diff --git a/tasks/sync_repo_languages.py b/tasks/sync_repo_languages.py index f402cd06b..d41aa9254 100644 --- a/tasks/sync_repo_languages.py +++ b/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 diff --git a/tasks/upload.py b/tasks/upload.py index c25429f4d..8ddd596d1 100644 --- a/tasks/upload.py +++ b/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 diff --git a/tasks/upload_processor.py b/tasks/upload_processor.py index 4636c7a34..e27cd023f 100644 --- a/tasks/upload_processor.py +++ b/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