Skip to content

Commit

Permalink
feat: use GithubAppInstalltion in the worker
Browse files Browse the repository at this point in the history
depends on: #236

context: codecov/engineering-team#970

usage of the `GithubAppInstallation` model through the worker.
It still respects and doesn't affect deprecated usages of
`owner.integration_id` and `repo.using_integration`, but if a
ghapp exists for the owner it takes precedence over the legacy fields.
  • Loading branch information
giovanni-guidini committed Feb 1, 2024
1 parent 451d112 commit a2016bc
Show file tree
Hide file tree
Showing 11 changed files with 429 additions and 21 deletions.
53 changes: 45 additions & 8 deletions services/bots.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from shared.torngit.base import TokenType

from database.models import Owner, Repository
from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
from helpers.environment import is_enterprise
from helpers.exceptions import OwnerWithoutValidBotError, RepositoryWithoutValidBotError
from services.encryption import encryptor
Expand All @@ -13,15 +14,46 @@
log = logging.getLogger(__name__)


def get_owner_installation_id(
owner: Owner,
deprecated_using_integration: bool,
*,
repository: Optional[Repository] = None,
installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME,
) -> Optional[int]:
default_app_installation_filter = filter(
lambda obj: obj.name == installation_name, owner.github_app_installations or []
)
# filter is an Iterator, so we need to scan matches
# in practice we only consider the 1st one
for app_installation in default_app_installation_filter:
if repository:
if app_installation.is_repo_covered_by_integration(repository):
return app_installation.installation_id
# The repo we want to get a token for is not covered by the installation
return None
else:
# Getting owner installation - not tied to any particular repo
return app_installation.installation_id
# DEPRECATED FLOW - begin
if owner.integration_id and deprecated_using_integration:
return owner.integration_id
# DEPRECATED FLOW - end
return None


def get_repo_appropriate_bot_token(repo: Repository) -> Tuple[Dict, Optional[Owner]]:
if is_enterprise() and get_config(repo.service, "bot"):
return get_public_bot_token(repo)
if repo.using_integration and repo.owner.integration_id:
github_token = get_github_integration_token(
repo.owner.service, repo.owner.integration_id
)

installation_id = get_owner_installation_id(
repo.owner, repo.using_integration, repository=repo
)
if installation_id:
github_token = get_github_integration_token(repo.owner.service, installation_id)
installation_token = dict(key=github_token)
# The token is not owned by an Owner object, so 2nd arg is None
return dict(key=github_token), None
return installation_token, None
try:
token_dict, appropriate_bot = get_repo_particular_bot_token(repo)
return token_dict, appropriate_bot
Expand Down Expand Up @@ -55,7 +87,10 @@ def get_repo_particular_bot_token(repo) -> Tuple[Dict, Owner]:
def get_token_type_mapping(repo: Repository):
if repo.private:
return None
if repo.using_integration and repo.owner.integration_id:
installation_id = get_owner_installation_id(
repo.owner, repo.using_integration, repository=repo
)
if installation_id:
return None
admin_bot = None
try:
Expand Down Expand Up @@ -101,9 +136,11 @@ def _get_repo_appropriate_bot(repo: Repository) -> Owner:


def get_owner_appropriate_bot_token(owner, using_integration) -> Dict:
if owner.integration_id and using_integration:
github_token = get_github_integration_token(owner.service, owner.integration_id)
installation_id = get_owner_installation_id(owner, using_integration)
if installation_id:
github_token = get_github_integration_token(owner.service, installation_id)
return dict(key=github_token)

return encryptor.decrypt_token(_get_owner_or_appropriate_bot(owner).oauth_token)


Expand Down
37 changes: 27 additions & 10 deletions services/notification/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from shared.helpers.yaml import default_if_true
from shared.yaml import UserYaml

from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
from helpers.metrics import metrics
from services.comparison.types import Comparison
from services.decoration import Decoration
Expand Down Expand Up @@ -48,6 +49,30 @@ def __init__(
self.current_yaml = current_yaml
self.decoration_type = decoration_type

def _should_use_checks_notifier(self) -> bool:
checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",))
if checks_yaml_field is False:
return False

owner = self.repository.owner
default_app_installation_filter = filter(
lambda obj: obj.name == GITHUB_APP_INSTALLATION_DEFAULT_NAME,
owner.github_app_installations or [],
)
# filter is an Iterator, so we need to scan matches
# in practice we only consider the 1st one
for app_installation in default_app_installation_filter:
return app_installation.is_repo_covered_by_integration(self.repository)
# DEPRECATED FLOW
return (
self.repository.using_integration
and self.repository.owner.integration_id
and (
self.repository.owner.service == "github"
or self.repository.owner.service == "github_enterprise"
)
)

def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]:
mapping = get_all_notifier_classes_mapping()
yaml_field = read_yaml_field(self.current_yaml, ("coverage", "notify"))
Expand All @@ -65,19 +90,11 @@ def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]:
current_yaml=self.current_yaml,
decoration_type=self.decoration_type,
)
checks_yaml_field = read_yaml_field(self.current_yaml, ("github_checks",))

current_flags = [rf.flag_name for rf in self.repository.flags if not rf.deleted]
for key, title, status_config in self.get_statuses(current_flags):
status_notifier_class = get_status_notifier_class(key, "status")
if (
self.repository.using_integration
and self.repository.owner.integration_id
and (
self.repository.owner.service == "github"
or self.repository.owner.service == "github_enterprise"
)
and checks_yaml_field is not False
):
if self._should_use_checks_notifier():
checks_notifier = get_status_notifier_class(key, "checks")
yield ChecksWithFallback(
checks_notifier=checks_notifier(
Expand Down
1 change: 1 addition & 0 deletions services/notification/notifiers/mixins/message/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ async def _possibly_write_gh_app_login_announcement(
if (
owner.service == "github"
and not owner.integration_id
and owner.github_app_installations == []
and not is_enterprise()
):
message_to_display = "Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality."
Expand Down
33 changes: 33 additions & 0 deletions services/notification/notifiers/tests/unit/test_comment.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from shared.yaml import UserYaml

import services.notification.notifiers.mixins.message.sections as sections
from database.models.core import GithubAppInstallation
from database.tests.factories import RepositoryFactory
from services.comparison.overlays.critical_path import CriticalPathOverlay
from services.decoration import Decoration
Expand Down Expand Up @@ -481,6 +482,38 @@ async def test__possibly_write_gh_app_login_announcement_already_using_integrati
)
assert mock_write.call_count == 0

@pytest.mark.asyncio
async def test__possibly_write_gh_app_login_announcement_has_ghapp_installation(
self, dbsession, mocker
):
repository = RepositoryFactory.create(
owner__service="github", owner__integration_id=None
)
ghapp_installation = GithubAppInstallation(
owner=repository.owner, installation_id=12345, repository_service_ids=None
)
dbsession.add_all([repository, ghapp_installation])
dbsession.flush()
mocker.patch(
"services.notification.notifiers.mixins.message.is_enterprise",
return_value=False,
)
mock_write = mocker.MagicMock()

notifier = CommentNotifier(
repository=repository,
title="some_title",
notifier_yaml_settings=False,
notifier_site_settings=None,
current_yaml={},
)
fake_comparison = mocker.MagicMock()
fake_comparison.head.commit.repository = repository
await notifier._possibly_write_gh_app_login_announcement(
fake_comparison, mock_write
)
assert mock_write.call_count == 0

@pytest.mark.asyncio
async def test__possibly_write_gh_app_login_announcement_enterprise(
self, dbsession, mocker
Expand Down
89 changes: 89 additions & 0 deletions services/notification/tests/unit/test_notification_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
from shared.yaml import UserYaml

from database.enums import Decoration, Notification, NotificationState
from database.models.core import (
GITHUB_APP_INSTALLATION_DEFAULT_NAME,
GithubAppInstallation,
)
from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory
from services.comparison import ComparisonProxy
from services.comparison.types import Comparison, EnrichedPull, FullCommit
Expand Down Expand Up @@ -47,6 +51,91 @@ def sample_comparison(dbsession, request):


class TestNotificationService(object):
def test_should_use_checks_notifier_yaml_field_false(self, dbsession):
repository = RepositoryFactory.create()
current_yaml = {"github_checks": False}
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == False

@pytest.mark.parametrize(
"repo_data,outcome",
[
(
dict(
using_integration=True,
owner__integration_id=12341234,
owner__service="github",
),
True,
),
(
dict(
using_integration=True,
owner__integration_id=12341234,
owner__service="gitlab",
),
False,
),
(
dict(
using_integration=True,
owner__integration_id=12341234,
owner__service="github_enterprise",
),
True,
),
(
dict(
using_integration=False,
owner__integration_id=None,
owner__service="github",
),
False,
),
],
)
def test_should_use_checks_notifier_deprecated_flow(
self, repo_data, outcome, dbsession
):
repository = RepositoryFactory.create(**repo_data)
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == []
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == outcome

def test_should_use_checks_notifier_ghapp_all_repos_covered(self, dbsession):
repository = RepositoryFactory.create()
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
installation_id=456789,
owner=repository.owner,
repository_service_ids=None,
)
dbsession.add(ghapp_installation)
dbsession.flush()
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == True

def test_should_use_checks_notifier_ghapp_some_repos_covered(self, dbsession):
repository = RepositoryFactory.create()
other_repo_same_owner = RepositoryFactory.create(owner=repository.owner)
ghapp_installation = GithubAppInstallation(
name=GITHUB_APP_INSTALLATION_DEFAULT_NAME,
installation_id=456789,
owner=repository.owner,
repository_service_ids=[repository.service_id],
)
dbsession.add(ghapp_installation)
dbsession.flush()
current_yaml = {"github_checks": True}
assert repository.owner.github_app_installations == [ghapp_installation]
service = NotificationService(repository, current_yaml)
assert service._should_use_checks_notifier() == True
service = NotificationService(other_repo_same_owner, current_yaml)
assert service._should_use_checks_notifier() == False

def test_get_notifiers_instances_only_third_party(
self, dbsession, mock_configuration
):
Expand Down
20 changes: 19 additions & 1 deletion services/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
from sqlalchemy.exc import IntegrityError

from database.models import Commit, Owner, Pull, Repository
from database.models.core import (
GITHUB_APP_INSTALLATION_DEFAULT_NAME,
GithubAppInstallation,
)
from helpers.token_refresh import get_token_refresh_callback
from services.bots import get_repo_appropriate_bot_token, get_token_type_mapping
from services.yaml import read_yaml_field
Expand All @@ -24,6 +28,20 @@
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 [],
)
)
if default_ghapp_installation:
ghapp_installation = owner.github_app_installations[0]
return ghapp_installation.is_repo_covered_by_integration(repo)
return repo.using_integration


def get_repo_provider_service(
repository, commit=None
) -> torngit.base.TorngitBaseAdapter:
Expand All @@ -36,7 +54,7 @@ def get_repo_provider_service(
adapter_params = dict(
repo=dict(
name=repository.name,
using_integration=repository.using_integration or False,
using_integration=_is_repo_using_integration(repository),
service_id=repository.service_id,
repoid=repository.repoid,
),
Expand Down
Loading

0 comments on commit a2016bc

Please sign in to comment.