From 8386477fa1204660d1e141bafc00962f65e5faa4 Mon Sep 17 00:00:00 2001 From: Gguidini Date: Wed, 22 May 2024 20:18:22 +0200 Subject: [PATCH] feat: add function to get specific app's details Adds `get_specific_github_app_details` function to bots service. This allows us to get `GithubInstallationInfo` for the app we want. It fails with a new exception if the app can't be found. This is because this feature will be used to get the app that made a comment or status check. If we don't use the same to edit said comment it won't work. On top of that we need to start saving the ID of the apps we selected for Torngit adapter instances --- database/models/core.py | 5 +++ helpers/exceptions.py | 4 ++ requirements.in | 2 +- requirements.txt | 4 +- services/bots/github_apps.py | 39 +++++++++++++++++ services/bots/tests/test_bots.py | 40 ++++++++++++++---- services/bots/tests/test_github_apps.py | 51 +++++++++++++++++++++++ services/tests/test_bots.py | 10 ++++- services/tests/test_owner_service.py | 7 +++- services/tests/test_repository_service.py | 8 +++- 10 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 services/bots/tests/test_github_apps.py diff --git a/database/models/core.py b/database/models/core.py index 631f15501..ca4194313 100644 --- a/database/models/core.py +++ b/database/models/core.py @@ -504,6 +504,11 @@ class CommitNotification(CodecovBaseModel): id_ = Column("id", types.BigInteger, primary_key=True) commit_id = Column(types.BigInteger, ForeignKey("commits.id")) + gh_app_id = Column( + types.BigInteger, + ForeignKey("codecov_auth_githubappinstallation.id"), + nullable=True, + ) notification_type = Column( postgresql.ENUM(Notification, values_callable=lambda x: [e.value for e in x]), nullable=False, diff --git a/helpers/exceptions.py b/helpers/exceptions.py index 666fff6b3..94d241b73 100644 --- a/helpers/exceptions.py +++ b/helpers/exceptions.py @@ -10,6 +10,10 @@ class RepositoryWithoutValidBotError(Exception): pass +class RequestedGithubAppNotFound(Exception): + pass + + class OwnerWithoutValidBotError(Exception): pass diff --git a/requirements.in b/requirements.in index 3cb3c1622..f1fba1b94 100644 --- a/requirements.in +++ b/requirements.in @@ -1,4 +1,4 @@ -https://github.com/codecov/shared/archive/4b51e8af9a42660c7c2f84831534349016ef5113.tar.gz#egg=shared +https://github.com/codecov/shared/archive/7a7847acc7ddb97c3c172e3a9231d74ae18241fc.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 aa7eb4845..5cc2b3eb6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,7 +2,7 @@ # This file is autogenerated by pip-compile with Python 3.12 # by the following command: # -# pip-compile requirements.in +# pip-compile # amqp==5.2.0 # via kombu @@ -368,7 +368,7 @@ sentry-sdk==1.40.0 # via # -r requirements.in # shared -shared @ https://github.com/codecov/shared/archive/4b51e8af9a42660c7c2f84831534349016ef5113.tar.gz +shared @ https://github.com/codecov/shared/archive/7a7847acc7ddb97c3c172e3a9231d74ae18241fc.tar.gz # via -r requirements.in six==1.16.0 # via diff --git a/services/bots/github_apps.py b/services/bots/github_apps.py index a73b105f0..3e8760ead 100644 --- a/services/bots/github_apps.py +++ b/services/bots/github_apps.py @@ -15,6 +15,7 @@ ) from helpers.exceptions import ( NoConfiguredAppsAvailable, + RequestedGithubAppNotFound, ) from services.bots.types import TokenWithOwner from services.github import get_github_integration_token @@ -131,6 +132,43 @@ def get_github_app_token( return installation_token, None +def get_specific_github_app_details( + owner: Owner, github_app_id: int, commitid: str +) -> GithubInstallationInfo: + """Gets the GithubInstallationInfo for GithubAppInstallation with id github_app_id. + + Args: + owner (Owner): the owner of the app. We look only in the apps for this owner. + github_app_id (int): the ID of the GithubAppInstallation we're looking for + commitid (str): Commit.commitid, used for logging purposes + + Raises: + RequestedGithubAppNotFound - if the app is not found in the given owner raise exception. + The assumption is that we need this specific app for a reason, and if we can't find the app + it's better to just fail + """ + app: GithubAppInstallation = next( + (obj for obj in owner.github_app_installations if obj.id == github_app_id), None + ) + if app is None: + log.exception( + "Can't find requested app", + extra=dict(ghapp_id=github_app_id, commitid=commitid), + ) + raise RequestedGithubAppNotFound() + if not app.is_configured(): + log.warning( + "Request for specific app that is not configured", + extra=dict(ghapp_id=id, commitid=commitid), + ) + return GithubInstallationInfo( + id=app.id, + installation_id=app.installation_id, + app_id=app.app_id, + pem_path=app.pem_path, + ) + + def get_github_app_info_for_owner( owner: Owner, *, @@ -203,6 +241,7 @@ def get_github_app_info_for_owner( info_to_get_tokens = list( map( lambda obj: GithubInstallationInfo( + id=obj.id, installation_id=obj.installation_id, app_id=obj.app_id, pem_path=obj.pem_path, diff --git a/services/bots/tests/test_bots.py b/services/bots/tests/test_bots.py index 6aee6d2b4..05e74ed29 100644 --- a/services/bots/tests/test_bots.py +++ b/services/bots/tests/test_bots.py @@ -114,7 +114,10 @@ def test_select_owner_single_installation(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ), fallback_installations=[], token_type_mapping=None, @@ -210,7 +213,10 @@ def test_select_owner_multiple_installations_default_name(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ), fallback_installations=[], token_type_mapping=None, @@ -250,11 +256,17 @@ def test_select_owner_multiple_installations_custom_name(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1300, app_id=300, pem_path="pem_path" + id=installations[1].id, + installation_id=1300, + app_id=300, + pem_path="pem_path", ), fallback_installations=[ GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ) ], token_type_mapping=None, @@ -437,7 +449,10 @@ def test_select_repo_single_installation(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ), fallback_installations=[], token_type_mapping=None, @@ -504,7 +519,10 @@ def test_select_repo_multiple_installations_default_name(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ), fallback_installations=[], token_type_mapping=None, @@ -547,11 +565,17 @@ def test_select_repo_multiple_installations_custom_name(self, dbsession): ), token_owner=None, selected_installation_info=GithubInstallationInfo( - installation_id=1300, app_id=300, pem_path="pem_path" + id=installations[1].id, + installation_id=1300, + app_id=300, + pem_path="pem_path", ), fallback_installations=[ GithubInstallationInfo( - installation_id=1200, app_id=200, pem_path="pem_path" + id=installations[0].id, + installation_id=1200, + app_id=200, + pem_path="pem_path", ) ], token_type_mapping=None, diff --git a/services/bots/tests/test_github_apps.py b/services/bots/tests/test_github_apps.py new file mode 100644 index 000000000..14239f969 --- /dev/null +++ b/services/bots/tests/test_github_apps.py @@ -0,0 +1,51 @@ +import pytest +from shared.typings.torngit import GithubInstallationInfo + +from database.models.core import GithubAppInstallation +from database.tests.factories.core import OwnerFactory +from helpers.exceptions import RequestedGithubAppNotFound +from services.bots.github_apps import get_specific_github_app_details + + +class TestGetSpecificGithubAppDetails(object): + def _get_owner_with_apps(self, dbsession): + owner = OwnerFactory(service="github") + app_1 = GithubAppInstallation( + owner=owner, + installation_id=1200, + app_id=12, + ) + app_2 = GithubAppInstallation( + owner=owner, + installation_id=1500, + app_id=15, + pem_path="some_path", + ) + dbsession.add_all([owner, app_1, app_2]) + dbsession.flush() + assert owner.github_app_installations == [app_1, app_2] + return owner + + def test_get_specific_github_app_details(self, dbsession): + owner = self._get_owner_with_apps(dbsession) + assert get_specific_github_app_details( + owner, owner.github_app_installations[0].id, "commit_id_for_logs" + ) == GithubInstallationInfo( + id=owner.github_app_installations[0].id, + installation_id=1200, + app_id=12, + pem_path=None, + ) + assert get_specific_github_app_details( + owner, owner.github_app_installations[1].id, "commit_id_for_logs" + ) == GithubInstallationInfo( + id=owner.github_app_installations[1].id, + installation_id=1500, + app_id=15, + pem_path="some_path", + ) + + def test_get_specific_github_app_not_found(self, dbsession): + owner = self._get_owner_with_apps(dbsession) + with pytest.raises(RequestedGithubAppNotFound): + get_specific_github_app_details(owner, 123456, "commit_id_for_logs") diff --git a/services/tests/test_bots.py b/services/tests/test_bots.py index c494fe25f..444c39916 100644 --- a/services/tests/test_bots.py +++ b/services/tests/test_bots.py @@ -289,6 +289,7 @@ def test_get_repo_appropriate_bot_token_via_installation_covered_repo( installations = get_github_app_info_for_owner(repo.owner) assert installations == [ { + "id": installation.id, "installation_id": 12341234, "app_id": None, "pem_path": None, @@ -360,6 +361,7 @@ def test_get_owner_installation_id_yes_installation_yes_legacy_integration( assert owner.github_app_installations == [installation] assert get_github_app_info_for_owner(owner) == [ { + "id": installation.id, "installation_id": 123456, "app_id": None, "pem_path": None, @@ -389,11 +391,17 @@ def test_get_owner_installation_id_yes_installation_yes_legacy_integration_yes_f assert owner.github_app_installations == [installation_0, installation_1] assert get_github_app_info_for_owner(owner, installation_name="my_app") == [ { + "id": installation_1.id, "installation_id": 12000, "app_id": 1212, "pem_path": "path", }, - {"installation_id": 123456, "app_id": None, "pem_path": None}, + { + "id": installation_0.id, + "installation_id": 123456, + "app_id": None, + "pem_path": None, + }, ] def test_get_owner_installation_id_yes_installation_all_rate_limited( diff --git a/services/tests/test_owner_service.py b/services/tests/test_owner_service.py index 4fff46370..f8176f369 100644 --- a/services/tests/test_owner_service.py +++ b/services/tests/test_owner_service.py @@ -57,7 +57,12 @@ def test_get_owner_provider_service_with_installation(self, dbsession, mocker): "username": owner.username, }, "repo": {}, - "installation": {"installation_id": 1500, "pem_path": None, "app_id": None}, + "installation": { + "id": installation.id, + "installation_id": 1500, + "pem_path": None, + "app_id": None, + }, "fallback_installations": [], } assert res.service == "github" diff --git a/services/tests/test_repository_service.py b/services/tests/test_repository_service.py index 8011efbf6..24fbb58e1 100644 --- a/services/tests/test_repository_service.py +++ b/services/tests/test_repository_service.py @@ -159,12 +159,18 @@ def test_get_repo_provider_service_github_with_installations( "repoid": repo.repoid, }, "installation": { + "id": installation_1.id, "installation_id": 1300, "app_id": 300, "pem_path": "path", }, "fallback_installations": [ - {"app_id": 200, "installation_id": 1200, "pem_path": None} + { + "id": installation_0.id, + "app_id": 200, + "installation_id": 1200, + "pem_path": None, + } ], } assert res.data == expected_data