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