diff --git a/conftest.py b/conftest.py index abfd10a65..def4c0370 100644 --- a/conftest.py +++ b/conftest.py @@ -212,9 +212,14 @@ def mock_repo_provider(mocker): @pytest.fixture def mock_owner_provider(mocker): - m = mocker.patch("services.owner._get_owner_provider_service_instance") provider_instance = mocker.MagicMock(GithubHandler) - m.return_value = provider_instance + + def side_effect(*args, **kwargs): + provider_instance.data = {**kwargs} + return provider_instance + + m = mocker.patch("services.owner._get_owner_provider_service_instance") + m.side_effect = side_effect yield provider_instance diff --git a/requirements.in b/requirements.in index ec86a988e..d385f1ef1 100644 --- a/requirements.in +++ b/requirements.in @@ -1,4 +1,4 @@ -https://github.com/codecov/shared/archive/8a2fec1557b2a36ac7a9c70f3fec9e37633a3a44.tar.gz#egg=shared +https://github.com/codecov/shared/archive/5df0e67bbe5093750a4bb48871e825b694d8f338.tar.gz#egg=shared https://github.com/codecov/opentelem-python/archive/refs/tags/v0.0.4a1.tar.gz#egg=codecovopentelem https://github.com/codecov/test-results-parser/archive/5515e960d5d38881036e9127f86320efca649f13.tar.gz#egg=test-results-parser boto3>=1.34 diff --git a/requirements.txt b/requirements.txt index 25b3184a7..76e6bc049 100644 --- a/requirements.txt +++ b/requirements.txt @@ -359,7 +359,9 @@ requests==2.31.0 respx==0.20.2 # via -r requirements.in rfc3986[idna2008]==1.4.0 - # via httpx + # via + # httpx + # rfc3986 rsa==4.7.2 # via google-auth s3transfer==0.10.1 @@ -370,7 +372,7 @@ sentry-sdk==1.40.0 # via # -r requirements.in # shared -shared @ https://github.com/codecov/shared/archive/8a2fec1557b2a36ac7a9c70f3fec9e37633a3a44.tar.gz +shared @ https://github.com/codecov/shared/archive/5df0e67bbe5093750a4bb48871e825b694d8f338.tar.gz # via -r requirements.in six==1.16.0 # via diff --git a/services/bots.py b/services/bots.py index 75609eab3..7dbab3a0c 100644 --- a/services/bots.py +++ b/services/bots.py @@ -6,7 +6,7 @@ from shared.config import get_config from shared.github import is_installation_rate_limited from shared.torngit.base import TokenType -from shared.typings.oauth_token_types import GithubInstallationInfo +from shared.typings.torngit import GithubInstallationInfo from database.models import Owner, Repository from database.models.core import ( @@ -127,7 +127,9 @@ def get_owner_installation_id( apps_matching_criteria_count = len(apps_to_consider) apps_to_consider = list( filter( - lambda obj: not is_installation_rate_limited(redis_connection, obj.id), + lambda obj: not is_installation_rate_limited( + redis_connection, obj.installation_id, app_id=obj.app_id + ), apps_to_consider, ) ) diff --git a/services/owner.py b/services/owner.py index 2dbbc6e02..d479e5c7c 100644 --- a/services/owner.py +++ b/services/owner.py @@ -2,6 +2,11 @@ import shared.torngit as torngit from shared.config import get_config, get_verify_ssl +from shared.typings.torngit import ( + GithubInstallationInfo, + OwnerInfo, + TorngitInstanceData, +) from helpers.token_refresh import get_token_refresh_callback from services.bots import get_owner_appropriate_bot_token, get_owner_installation_id @@ -21,10 +26,22 @@ def get_owner_provider_service( owner, using_integration, ignore_installation=ignore_installation ) token = get_owner_appropriate_bot_token(owner, installation_dict=installation_info) - adapter_params = dict( - owner=dict( + data = TorngitInstanceData( + owner=OwnerInfo( service_id=owner.service_id, ownerid=owner.ownerid, username=owner.username ), + installation=None, + fallback_installations=None, + ) + if installation_info: + data["installation"] = GithubInstallationInfo( + installation_id=installation_info.get("installation_id"), + app_id=installation_info.get("app_id"), + pem_path=installation_info.get("pem_path"), + ) + data["fallback_installations"] = installation_info.get("fallback_installations") + + adapter_params = dict( token=token, verify_ssl=get_verify_ssl(service), timeouts=_timeouts, @@ -37,9 +54,7 @@ def get_owner_provider_service( on_token_refresh=( get_token_refresh_callback(owner) if not using_integration else None ), - fallback_installations=installation_info.get("fallback_installations") - if installation_info - else None, + **data ) return _get_owner_provider_service_instance(service, **adapter_params) diff --git a/services/repository.py b/services/repository.py index 3c1ecfef5..ddbc7a640 100644 --- a/services/repository.py +++ b/services/repository.py @@ -11,6 +11,12 @@ TorngitError, TorngitObjectNotFoundError, ) +from shared.typings.torngit import ( + GithubInstallationInfo, + OwnerInfo, + RepoInfo, + TorngitInstanceData, +) from sqlalchemy.dialects.postgresql import insert from sqlalchemy.exc import IntegrityError @@ -64,18 +70,30 @@ def get_repo_provider_service( installation_name=installation_name_to_use, ) token, token_owner = get_repo_appropriate_bot_token(repository, installation_info) - adapter_params = dict( - repo=dict( + data = TorngitInstanceData( + repo=RepoInfo( name=repository.name, using_integration=_is_repo_using_integration(repository), service_id=repository.service_id, repoid=repository.repoid, ), - owner=dict( + owner=OwnerInfo( service_id=repository.owner.service_id, ownerid=repository.ownerid, username=repository.owner.username, ), + installation=None, + fallback_installations=None, + ) + if installation_info: + data["installation"] = GithubInstallationInfo( + installation_id=installation_info.get("installation_id"), + app_id=installation_info.get("app_id"), + pem_path=installation_info.get("pem_path"), + ) + data["fallback_installations"] = installation_info.get("fallback_installations") + + adapter_params = dict( token=token, token_type_mapping=get_token_type_mapping( repository, installation_name=installation_name_to_use @@ -87,11 +105,7 @@ def get_repo_provider_service( secret=get_config(service, "client_secret"), ), on_token_refresh=get_token_refresh_callback(token_owner), - fallback_installations=( - installation_info.get("fallback_installations") - if installation_info - else None - ), + **data, ) return _get_repo_provider_service_instance(repository.service, **adapter_params) diff --git a/services/tests/test_bots.py b/services/tests/test_bots.py index 0494e19bc..b0dbe954d 100644 --- a/services/tests/test_bots.py +++ b/services/tests/test_bots.py @@ -482,11 +482,9 @@ def test_get_owner_installation_id_yes_installation_all_rate_limited( with pytest.raises(NoConfiguredAppsAvailable): get_owner_installation_id(owner, True, installation_name="my_app") mock_redis.exists.assert_any_call( - f"rate_limited_installations_{installation_0.id}" - ) - mock_redis.exists.assert_any_call( - f"rate_limited_installations_{installation_1.id}" + f"rate_limited_installations_default_app_123456" ) + mock_redis.exists.assert_any_call(f"rate_limited_installations_1212_12000") def test_get_owner_installation_id_yes_installation_yes_legacy_integration_specific_repos( self, mocker, dbsession diff --git a/services/tests/test_owner_service.py b/services/tests/test_owner_service.py index 168fb42eb..85419032d 100644 --- a/services/tests/test_owner_service.py +++ b/services/tests/test_owner_service.py @@ -1,5 +1,9 @@ import pytest +from database.models.core import ( + GITHUB_APP_INSTALLATION_DEFAULT_NAME, + GithubAppInstallation, +) from database.tests.factories import OwnerFactory from services.owner import get_owner_provider_service @@ -21,12 +25,47 @@ def test_get_owner_provider_service(self, dbsession): "username": owner.username, }, "repo": {}, + "installation": None, "fallback_installations": None, } assert res.service == "github" assert res.data == expected_data assert res.token == {"key": "bcaa0dc0c66b4a8c8c65ac919a1a91aa", "secret": None} + def test_get_owner_provider_service_with_installation(self, dbsession, mocker): + mocker.patch( + "services.bots.get_github_integration_token", + return_value="integration_token", + ) + owner = OwnerFactory.create( + service="github", + unencrypted_oauth_token="bcaa0dc0c66b4a8c8c65ac919a1a91aa", + bot=None, + ) + dbsession.add(owner) + installation = GithubAppInstallation( + name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, + installation_id=1500, + repository_service_ids=None, + owner=owner, + ) + dbsession.add(installation) + dbsession.flush() + res = get_owner_provider_service(owner) + expected_data = { + "owner": { + "ownerid": owner.ownerid, + "service_id": owner.service_id, + "username": owner.username, + }, + "repo": {}, + "installation": {"installation_id": 1500, "pem_path": None, "app_id": None}, + "fallback_installations": [], + } + assert res.service == "github" + assert res.data == expected_data + assert res.token == {"key": "integration_token"} + def test_get_owner_provider_service_other_service(self, dbsession): owner = OwnerFactory.create( service="gitlab", unencrypted_oauth_token="testenll80qbqhofao65", bot=None @@ -41,6 +80,7 @@ def test_get_owner_provider_service_other_service(self, dbsession): "username": owner.username, }, "repo": {}, + "installation": None, "fallback_installations": None, } assert res.service == "gitlab" @@ -63,6 +103,7 @@ def test_get_owner_provider_service_different_bot(self, dbsession): "username": owner.username, }, "repo": {}, + "installation": None, "fallback_installations": None, } assert res.data["repo"] == expected_data["repo"] diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index b54b8cd7b..443a33676 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -102,6 +102,7 @@ def test_get_repo_provider_service_github(self, dbsession): "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data == expected_data @@ -157,6 +158,11 @@ def test_get_repo_provider_service_github_with_installations( "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": { + "installation_id": 1300, + "app_id": 300, + "pem_path": "path", + }, "fallback_installations": [ {"app_id": 200, "installation_id": 1200, "pem_path": None} ], @@ -189,6 +195,7 @@ def test_get_repo_provider_service_bitbucket(self, dbsession): "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data == expected_data @@ -221,6 +228,7 @@ def test_get_repo_provider_service_with_token_refresh_callback(self, dbsession): "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data == expected_data @@ -254,6 +262,7 @@ def test_get_repo_provider_service_repo_bot(self, dbsession, mock_configuration) "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data == expected_data @@ -306,6 +315,7 @@ def test_get_repo_provider_service_different_bot(self, dbsession): "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data["repo"] == expected_data["repo"] @@ -341,6 +351,7 @@ def test_get_repo_provider_service_no_bot(self, dbsession): "service_id": repo.service_id, "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data == expected_data @@ -1080,6 +1091,7 @@ async def test_get_repo_gh_no_integration(self, dbsession, mocker): "service_id": "123456", "repoid": repo.repoid, }, + "installation": None, "fallback_installations": None, } assert res.data["repo"] == expected_data["repo"] diff --git a/tasks/sync_repos.py b/tasks/sync_repos.py index 9d8556a26..41a17ccf4 100644 --- a/tasks/sync_repos.py +++ b/tasks/sync_repos.py @@ -12,6 +12,7 @@ ) from shared.config import get_config from shared.metrics import metrics +from shared.torngit.base import TorngitBaseAdapter from shared.torngit.exceptions import TorngitClientError from sqlalchemy import and_ from sqlalchemy.orm.session import Session @@ -134,10 +135,10 @@ def run_impl( async def sync_repos_affected_repos_known( self, - db_session, - git, + db_session: Session, + git: TorngitBaseAdapter, owner: Owner, - repository_service_ids: Optional[List[Tuple[int, str]]], + repository_service_ids: List[Tuple[int, str]] | None, ): repoids_added = [] # Casting to str in case celery interprets the service ID as a integer for some reason @@ -199,8 +200,16 @@ async def sync_repos_affected_repos_known( return repoids_added def _possibly_update_ghinstallation_covered_repos( - self, db_session, git, owner: Owner, service_ids_listed: List[str] + self, + git: TorngitBaseAdapter, + owner: Owner, + service_ids_listed: List[str], ): + installation_used = git.data.get("installation") + if installation_used is None: + log.warning( + "Failed to update ghapp covered repos. We don't know which installation is being used" + ) if ( owner.github_app_installations is None or owner.github_app_installations == [] @@ -209,10 +218,16 @@ def _possibly_update_ghinstallation_covered_repos( "Failed to possibly update ghapp covered repos. Owner has no installations", ), return - # FIXME This is no *always* true, but in general owners have 1 installation, - # That is for the default app. So it's OK for a quick fix. - # The more elaborate fix involves passing more info to the `git` object and get the installation being used from that. - ghapp = owner.github_app_installations[0] + ghapp = next( + filter( + lambda obj: ( + obj.installation_id == installation_used.get("installation_id") + and obj.app_id == installation_used.get("app_id") + ), + owner.github_app_installations, + ), + None, + ) if ghapp and ghapp.repository_service_ids is not None: covered_repos = set(ghapp.repository_service_ids) service_ids_listed_set = set(service_ids_listed) @@ -232,7 +247,7 @@ def _possibly_update_ghinstallation_covered_repos( async def sync_repos_using_integration( self, db_session: Session, - git, + git: TorngitBaseAdapter, owner: Owner, username: str, repository_service_ids: Optional[List[Tuple[int, str]]] = None, @@ -249,9 +264,7 @@ async def sync_repos_using_integration( # function avoids duplicating the code in the old and new paths def process_repos(repos): service_ids = {repo["repo"]["service_id"] for repo in repos} - self._possibly_update_ghinstallation_covered_repos( - db_session, git, owner, service_ids - ) + self._possibly_update_ghinstallation_covered_repos(git, owner, service_ids) if service_ids: # Querying through the `Repository` model is cleaner, but we # need to go through the table object instead if we want to