From 28002130f07229e4d91d4a8dadf894202c3b2911 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Mon, 30 Sep 2024 17:13:56 +0200 Subject: [PATCH] Revert "Simplifies `get_installation_name` a bit (#749)" This reverts commit 76070910efd11807e94cdd14ef9af743346ab4f7. --- 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, 42 insertions(+), 40 deletions(-) diff --git a/helpers/github_installation.py b/helpers/github_installation.py index 202ede12d..ebe0f7b0d 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__) -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() +@cache.cache_function(ttl=86400) # 1 day +def get_installation_name_for_owner_for_task( + dbsession: Session, task_name: str, owner: Owner +) -> str: 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 7b9170096..8ff5f0896 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(service="github") + owner = OwnerFactory() other_owner = OwnerFactory() task_name = "app.tasks.notify.Notify" installation_task_config = OwnerInstallationNameToUseForTask( @@ -21,13 +21,14 @@ 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(task_name, owner) == "my_installation" + get_installation_name_for_owner_for_task(dbsession, task_name, owner) + == "my_installation" ) assert ( - get_installation_name_for_owner_for_task(task_name, other_owner) + get_installation_name_for_owner_for_task(dbsession, task_name, other_owner) == GITHUB_APP_INSTALLATION_DEFAULT_NAME ) assert ( - get_installation_name_for_owner_for_task("other_task", owner) + get_installation_name_for_owner_for_task(dbsession, "other_task", owner) == GITHUB_APP_INSTALLATION_DEFAULT_NAME ) diff --git a/services/repository.py b/services/repository.py index fb9536cac..33a0f6760 100644 --- a/services/repository.py +++ b/services/repository.py @@ -31,7 +31,6 @@ 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 @@ -46,7 +45,7 @@ def get_repo_provider_service_for_specific_commit( commit: Commit, fallback_installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, -) -> TorngitBaseAdapter: +) -> torngit.base.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 @@ -85,16 +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 -@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: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, + installation_name_to_use: Optional[str] = GITHUB_APP_INSTALLATION_DEFAULT_NAME, ) -> TorngitBaseAdapter: adapter_auth_info = get_adapter_auth_information( repository.owner, @@ -125,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: dict): +def _get_repo_provider_service_instance(service: str, **adapter_params): _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 97fafca91..b56c4b321 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -1178,12 +1178,10 @@ def test_get_repo_provider_service_for_specific_commit( ) mock_get_instance.assert_called_with( "github", - dict( - **data, - token="the app token", - token_type_mapping=None, - on_token_refresh=None, - ), + **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 d57f5cd0b..086030b5b 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( - self.name, commit.repository.owner + db_session, 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 fb30ed9b4..284273b63 100644 --- a/tasks/commit_update.py +++ b/tasks/commit_update.py @@ -2,7 +2,10 @@ 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 @@ -36,7 +39,7 @@ def run_impl( was_updated = False try: installation_name_to_use = get_installation_name_for_owner_for_task( - self.name, repository.owner + db_session, 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 69ae83be9..461e3642b 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( - self.name, repo.owner + db_session, self.name, repo.owner ) comparison_proxy = self.get_comparison_proxy( diff --git a/tasks/notify.py b/tasks/notify.py index 159a1c4f4..8c9b64ca5 100644 --- a/tasks/notify.py +++ b/tasks/notify.py @@ -197,9 +197,11 @@ def run_impl_within_lock( } try: - installation_name_to_use = get_installation_name_for_owner_for_task( - self.name, commit.repository.owner - ) + 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 + ) 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 3417f125c..cc38522f5 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( - self.name, commit.repository.owner + db_session, 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 b66148ce9..a4498e2f2 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( - self.name, commit.repository.owner + db_session, 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 a8ffeeb10..23fdd64a9 100644 --- a/tasks/sync_pull.py +++ b/tasks/sync_pull.py @@ -21,7 +21,9 @@ 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 @@ -111,7 +113,7 @@ def run_impl_within_lock( assert repository try: installation_name_to_use = get_installation_name_for_owner_for_task( - self.name, repository.owner + db_session, 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 d41aa9254..f402cd06b 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( - self.name, repository.owner + db_session, 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 8ddd596d1..c25429f4d 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( - self.name, repository.owner + db_session, 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 e27cd023f..4636c7a34 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( - self.name, repository.owner + db_session, self.name, repository.owner ) repository_service = get_repo_provider_service( repository, installation_name_to_use=installation_name_to_use