From 3cdeed1bbd2d0baeb431d10e7a36748f4d3dbd03 Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini <99758426+giovanni-guidini@users.noreply.github.com> Date: Mon, 3 Jun 2024 12:12:34 +0200 Subject: [PATCH] feat: get repo service for specific commit (#479) --- services/github.py | 3 + services/repository.py | 92 +++++++++---- services/tests/test_github.py | 30 ++++- services/tests/test_repository_service.py | 153 +++++++++++++++------- 4 files changed, 202 insertions(+), 76 deletions(-) diff --git a/services/github.py b/services/github.py index 2d52cd86e..2dff07d34 100644 --- a/services/github.py +++ b/services/github.py @@ -64,6 +64,9 @@ def set_github_app_for_commit( def get_github_app_for_commit(commit: Commit) -> str | None: + if commit.repository.service not in ["github", "github_enterprise"]: + # Because this feature is GitHub-exclusive we can skip checking for other providers + return None redis = get_redis_connection() try: return redis.get(COMMIT_GHAPP_KEY_NAME(commit.id)) diff --git a/services/repository.py b/services/repository.py index e7fe2c58a..175310ddc 100644 --- a/services/repository.py +++ b/services/repository.py @@ -6,6 +6,7 @@ import shared.torngit as torngit from shared.config import get_config, get_verify_ssl +from shared.django_apps.codecov_auth.models import Service from shared.torngit.exceptions import ( TorngitClientError, TorngitError, @@ -27,6 +28,11 @@ from services.bots import ( get_adapter_auth_information, ) +from services.bots.github_apps import ( + get_github_app_token, + get_specific_github_app_details, +) +from services.github import get_github_app_for_commit from services.yaml import read_yaml_field log = logging.getLogger(__name__) @@ -34,29 +40,55 @@ merged_pull = re.compile(r".*Merged in [^\s]+ \(pull request \#(\d+)\).*").match -def _is_repo_using_integration(repo: Repository) -> bool: - owner = repo.owner - default_ghapp_installation = list( - filter( - lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME, - owner.github_app_installations or [], - ) +def get_repo_provider_service_for_specific_commit( + commit: Commit, + fallback_installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, +) -> 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 + + This is done specifically after emitting checks for a PR using GitHub apps, because only the app + that posted the check can edit it later on. The "app for a commit" info is saved in Redis by the NotifyTask. + """ + repository = commit.repository + installation_for_commit = get_github_app_for_commit(commit) + if installation_for_commit is None: + return get_repo_provider_service(repository, fallback_installation_name) + + ghapp_details = get_specific_github_app_details( + repository.owner, int(installation_for_commit), commit.commitid + ) + token, _ = get_github_app_token(Service(repository.service), ghapp_details) + + data = TorngitInstanceData( + repo=RepoInfo( + name=repository.name, + using_integration=True, + service_id=repository.service_id, + repoid=repository.repoid, + ), + owner=OwnerInfo( + service_id=repository.owner.service_id, + ownerid=repository.ownerid, + username=repository.owner.username, + ), + installation=ghapp_details, + fallback_installations=None, ) - if default_ghapp_installation: - ghapp_installation = owner.github_app_installations[0] - return ghapp_installation.is_repo_covered_by_integration(repo) - return repo.using_integration + + adapter_params = dict( + token=token, + token_type_mapping=None, + on_token_refresh=None, + **data, + ) + return _get_repo_provider_service_instance(repository.service, **adapter_params) def get_repo_provider_service( repository: Repository, installation_name_to_use: Optional[str] = GITHUB_APP_INSTALLATION_DEFAULT_NAME, ) -> torngit.base.TorngitBaseAdapter: - _timeouts = [ - get_config("setup", "http", "timeouts", "connect", default=30), - get_config("setup", "http", "timeouts", "receive", default=60), - ] - service = repository.owner.service adapter_auth_info = get_adapter_auth_information( repository.owner, repository=repository, @@ -65,7 +97,9 @@ def get_repo_provider_service( data = TorngitInstanceData( repo=RepoInfo( name=repository.name, - using_integration=_is_repo_using_integration(repository), + using_integration=( + adapter_auth_info.get("selected_installation_info") is not None + ), service_id=repository.service_id, repoid=repository.repoid, ), @@ -81,20 +115,28 @@ def get_repo_provider_service( adapter_params = dict( token=adapter_auth_info["token"], token_type_mapping=adapter_auth_info["token_type_mapping"], - verify_ssl=get_verify_ssl(service), - timeouts=_timeouts, - oauth_consumer_token=dict( - key=get_config(service, "client_id"), - secret=get_config(service, "client_secret"), - ), on_token_refresh=get_token_refresh_callback(adapter_auth_info["token_owner"]), **data, ) return _get_repo_provider_service_instance(repository.service, **adapter_params) -def _get_repo_provider_service_instance(service_name, **adapter_params): - return torngit.get(service_name, **adapter_params) +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), + ] + return torngit.get( + service, + # Args for the Torngit instance + timeouts=_timeouts, + verify_ssl=get_verify_ssl(service), + oauth_consumer_token=dict( + key=get_config(service, "client_id"), + secret=get_config(service, "client_secret"), + ), + **adapter_params, + ) async def fetch_appropriate_parent_for_commit( diff --git a/services/tests/test_github.py b/services/tests/test_github.py index 088aa36bf..4e04879c8 100644 --- a/services/tests/test_github.py +++ b/services/tests/test_github.py @@ -4,7 +4,7 @@ from redis import RedisError from database.models.core import GithubAppInstallation, Owner -from database.tests.factories.core import CommitFactory +from database.tests.factories.core import CommitFactory, RepositoryFactory from services.github import get_github_app_for_commit, set_github_app_for_commit @@ -51,21 +51,39 @@ def test_set_app_for_commit_redis_error(self, mock_redis, dbsession): f"app_to_use_for_commit_{commit.id}", "1000", ex=(60 * 60 * 2) ) - def test_get_app_for_commit(self, mock_redis): + def test_get_app_for_commit(self, mock_redis, dbsession): + repo_github = RepositoryFactory(owner__service="github") + repo_ghe = RepositoryFactory(owner__service="github_enterprise") + repo_gitlab = RepositoryFactory(owner__service="gitlab") redis_keys = { "app_to_use_for_commit_12": "1200", "app_to_use_for_commit_10": "1000", } - fake_commit_12 = MagicMock(name="fake_commit", **{"id": 12}) - fake_commit_10 = MagicMock(name="fake_commit", **{"id": 10}) - fake_commit_50 = MagicMock(name="fake_commit", **{"id": 50}) + fake_commit_12 = MagicMock( + name="fake_commit", **{"id": 12, "repository": repo_github} + ) + fake_commit_10 = MagicMock( + name="fake_commit", + **{"id": 10, "repository": repo_ghe}, + ) + fake_commit_50 = MagicMock( + name="fake_commit", **{"id": 50, "repository": repo_github} + ) + fake_commit_gitlab = MagicMock( + name="fake_commit", **{"id": 12, "repository": repo_gitlab} + ) mock_redis.get.side_effect = lambda key: redis_keys.get(key) assert get_github_app_for_commit(fake_commit_12) == "1200" assert get_github_app_for_commit(fake_commit_10) == "1000" assert get_github_app_for_commit(fake_commit_50) == None + # This feature is Github-exclusive, so we skip checking for commits that are in repos of other providers + assert get_github_app_for_commit(fake_commit_gitlab) == None def test_get_app_for_commit_error(self, mock_redis): + repo_github = RepositoryFactory(owner__service="github") mock_redis.get.side_effect = RedisError - fake_commit_12 = MagicMock(name="fake_commit", **{"id": 12}) + fake_commit_12 = MagicMock( + name="fake_commit", **{"id": 12, "repository": repo_github} + ) assert get_github_app_for_commit(fake_commit_12) == None mock_redis.get.assert_called_with("app_to_use_for_commit_12") diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index 24fbb58e1..b23c93c8d 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -1,5 +1,6 @@ import inspect from datetime import datetime +from unittest.mock import MagicMock, patch import mock import pytest @@ -11,6 +12,12 @@ TorngitObjectNotFoundError, TorngitServerUnreachableError, ) +from shared.typings.torngit import ( + GithubInstallationInfo, + OwnerInfo, + RepoInfo, + TorngitInstanceData, +) from database.models import Owner from database.models.core import ( @@ -24,7 +31,6 @@ RepositoryFactory, ) from services.repository import ( - _is_repo_using_integration, _pick_best_base_comparedto_pair, fetch_and_update_pull_request_information, fetch_and_update_pull_request_information_from_commit, @@ -32,54 +38,11 @@ get_or_create_author, get_repo_provider_service, get_repo_provider_service_by_id, + get_repo_provider_service_for_specific_commit, update_commit_from_provider_info, ) -@pytest.mark.parametrize("using_integration", [True, False]) -def test__is_repo_using_integration_deprecated_flow(using_integration, dbsession): - repo = RepositoryFactory.create(using_integration=using_integration) - assert _is_repo_using_integration(repo) == using_integration - - -def test__is_repo_using_integration_ghapp_covers_all_repos(dbsession): - owner = OwnerFactory.create(service="github") - repo = RepositoryFactory.create(owner=owner) - other_repo_same_owner = RepositoryFactory.create(owner=owner) - repo_different_owner = RepositoryFactory.create() - assert repo.owner != repo_different_owner.owner - ghapp_installation = GithubAppInstallation( - name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, - owner=owner, - repository_service_ids=None, - installation_id=12345, - ) - dbsession.add(ghapp_installation) - dbsession.flush() - assert _is_repo_using_integration(repo) == True - assert _is_repo_using_integration(other_repo_same_owner) == True - assert _is_repo_using_integration(repo_different_owner) == False - - -def test__is_repo_using_integration_ghapp_covers_some_repos(dbsession): - owner = OwnerFactory.create(service="github") - repo = RepositoryFactory.create(owner=owner) - other_repo_same_owner = RepositoryFactory.create(owner=owner) - repo_different_owner = RepositoryFactory.create() - assert repo.owner != repo_different_owner.owner - ghapp_installation = GithubAppInstallation( - name=GITHUB_APP_INSTALLATION_DEFAULT_NAME, - owner=owner, - repository_service_ids=[repo.service_id], - installation_id=12345, - ) - dbsession.add(ghapp_installation) - dbsession.flush() - assert _is_repo_using_integration(repo) == True - assert _is_repo_using_integration(other_repo_same_owner) == False - assert _is_repo_using_integration(repo_different_owner) == False - - class TestRepositoryServiceTestCase(object): def test_get_repo_provider_service_github(self, dbsession): repo = RepositoryFactory.create( @@ -1109,6 +1072,106 @@ async def test_get_repo_gh_no_integration(self, dbsession, mocker): } +class TestGetRepoProviderServiceForSpecificCommit(object): + @pytest.fixture + def mock_get_repo_provider_service(self, mocker): + mock_get_repo_provider_service = mocker.patch( + "services.repository.get_repo_provider_service" + ) + return mock_get_repo_provider_service + + @pytest.fixture + def mock_redis(self, mocker): + fake_redis = MagicMock(name="fake_redis") + mock_conn = mocker.patch("services.github.get_redis_connection") + mock_conn.return_value = fake_redis + return fake_redis + + def test_get_repo_provider_service_for_specific_commit_not_gh( + self, dbsession, mock_get_repo_provider_service, mock_redis + ): + commit = CommitFactory(repository__owner__service="gitlab") + mock_get_repo_provider_service.return_value = "the TorngitAdapter" + response = get_repo_provider_service_for_specific_commit(commit, "some_name") + assert response == "the TorngitAdapter" + mock_get_repo_provider_service.assert_called_with( + commit.repository, "some_name" + ) + + def test_get_repo_provider_service_for_specific_commit_no_specific_app_for_commit( + self, dbsession, mock_get_repo_provider_service, mock_redis + ): + commit = CommitFactory(repository__owner__service="github") + assert commit.id not in [10000, 15000] + redis_keys = { + "app_to_use_for_commit_15000": "1200", + "app_to_use_for_commit_10000": "1000", + } + mock_redis.get.side_effect = lambda key: redis_keys.get(key) + + mock_get_repo_provider_service.return_value = "the TorngitAdapter" + response = get_repo_provider_service_for_specific_commit(commit, "some_name") + assert response == "the TorngitAdapter" + mock_get_repo_provider_service.assert_called_with( + commit.repository, "some_name" + ) + + @patch( + "services.repository.get_github_app_token", return_value=("the app token", None) + ) + @patch( + "services.repository._get_repo_provider_service_instance", + return_value="the TorngitAdapter", + ) + def test_get_repo_provider_service_for_specific_commit( + self, + mock_get_instance, + mock_get_app_token, + dbsession, + mock_get_repo_provider_service, + mock_redis, + ): + commit = CommitFactory(repository__owner__service="github") + app = GithubAppInstallation( + owner=commit.repository.owner, app_id=12, installation_id=1200 + ) + dbsession.add_all([commit, app]) + dbsession.flush() + assert commit.repository.owner.github_app_installations == [app] + redis_keys = { + f"app_to_use_for_commit_{commit.id}": str(app.id), + } + mock_redis.get.side_effect = lambda key: redis_keys.get(key) + response = get_repo_provider_service_for_specific_commit(commit, "some_name") + assert response == "the TorngitAdapter" + mock_get_instance.assert_called_once() + + data = TorngitInstanceData( + repo=RepoInfo( + name=commit.repository.name, + using_integration=True, + service_id=commit.repository.service_id, + repoid=commit.repository.repoid, + ), + owner=OwnerInfo( + service_id=commit.repository.owner.service_id, + ownerid=commit.repository.ownerid, + username=commit.repository.owner.username, + ), + installation=GithubInstallationInfo( + id=app.id, app_id=12, installation_id=1200, pem_path=None + ), + fallback_installations=None, + ) + mock_get_instance.assert_called_with( + "github", + **data, + token="the app token", + token_type_mapping=None, + on_token_refresh=None, + ) + + class TestPullRequestFetcher(object): @pytest.mark.asyncio async def test_fetch_and_update_pull_request_information_from_commit_new_pull_commits_in_place(