Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: get repo service for specific commit #479

Merged
merged 2 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions services/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
92 changes: 67 additions & 25 deletions services/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -27,36 +28,67 @@
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__)

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the logic inside here,

, can you still use your app ifit is not configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do a check in get_repo_provider_service to ensure that the repo is a GitHub repo. Should we do a check here as well? This will save us a database read to the GitHub App Installation table I think.

In fact, many parts of this function is GitHub specific... can we skip some more steps below if it's not a GitHub commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is in get_github_app_for_commit.

From the docstring: "If the commit doesn't have a particular app assigned to it, return regular get_repo_provider_service choice".
Because apps only exist for GitHub it follows that only if it's a GitHub repo the code would get to this point.

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,
Expand All @@ -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,
),
Expand All @@ -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(
Expand Down
30 changes: 24 additions & 6 deletions services/tests/test_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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")
153 changes: 108 additions & 45 deletions services/tests/test_repository_service.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import inspect
from datetime import datetime
from unittest.mock import MagicMock, patch

import mock
import pytest
Expand All @@ -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 (
Expand All @@ -24,62 +31,18 @@
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,
fetch_appropriate_parent_for_commit,
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(
Expand Down Expand Up @@ -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(
Expand Down
Loading