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: don't select suspended apps #534

Merged
merged 4 commits into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 2 additions & 0 deletions database/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ class GithubAppInstallation(CodecovBaseModel, MixinBaseClass):
Owner, foreign_keys=[ownerid], back_populates="github_app_installations"
)

is_suspended = Column(types.Boolean, server_default=FetchedValue())

def repository_queryset(self, dbsession: Session):
"""Returns a query set of repositories covered by this installation"""
if self.repository_service_ids is None:
Expand Down
7 changes: 5 additions & 2 deletions helpers/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ class OwnerWithoutValidBotError(Exception):


class NoConfiguredAppsAvailable(Exception):
def __init__(self, apps_count: int, all_rate_limited: bool) -> None:
def __init__(
self, apps_count: int, rate_limited_count: int, suspended_count: int
) -> None:
self.apps_count = apps_count
self.all_rate_limited = all_rate_limited
self.rate_limited_count = rate_limited_count
self.suspended_count = suspended_count


class CorruptRawReportError(Exception):
Expand Down
43 changes: 32 additions & 11 deletions services/bots/github_apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,26 @@ def get_specific_github_app_details(
)


def _filter_rate_limited_apps(
apps_to_consider: List[GithubAppInstallation],
) -> List[GithubAppInstallation]:
redis_connection = get_redis_connection()
return list(
filter(
lambda obj: not is_installation_rate_limited(
redis_connection, obj.installation_id, app_id=obj.app_id
),
apps_to_consider,
)
)


def _filter_suspended_apps(
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: making tests for this that isn't necessarily tethered to the notify test

apps_to_consider: List[GithubAppInstallation],
) -> List[GithubAppInstallation]:
return list(filter(lambda obj: not obj.is_suspended, apps_to_consider))


def get_github_app_info_for_owner(
owner: Owner,
*,
Expand Down Expand Up @@ -225,18 +245,15 @@ def get_github_app_info_for_owner(
owner, installation_name, repository
)
apps_matching_criteria_count = len(apps_to_consider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does apps_to_consider have a hierarchy of which app to try first over another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, as documented in the function that produces this list:

"""This function returns an ordered list of GithubAppInstallations that can be used to communicate with GitHub

The ordering of apps in the list is the hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see I see


redis_connection = get_redis_connection()
apps_to_consider = list(
filter(
lambda obj: not is_installation_rate_limited(
redis_connection, obj.installation_id, app_id=obj.app_id
),
apps_to_consider,
)
)
# We can't use apps that are rate limited
apps_to_consider = _filter_rate_limited_apps(apps_to_consider)
rate_limited_apps_count = apps_matching_criteria_count - len(apps_to_consider)
# We can't use apps that are suspended (by the user)
apps_to_consider = _filter_suspended_apps(apps_to_consider)
suspended_apps_count = rate_limited_apps_count - len(apps_to_consider)

if apps_to_consider:
# There's at least 1 app that matches all the criteria and can be used to communicate with GitHub
main_name = apps_to_consider[0].name
info_to_get_tokens = list(
map(
Expand All @@ -261,8 +278,12 @@ def get_github_app_info_for_owner(
)
return info_to_get_tokens
elif apps_matching_criteria_count > 0:
# There are apps that match the criteria, but we can't use them.
# Either they are currently rate limited or they have been suspended.
raise NoConfiguredAppsAvailable(
apps_count=apps_matching_criteria_count, all_rate_limited=True
apps_count=apps_matching_criteria_count,
rate_limited_count=rate_limited_apps_count,
suspended_count=suspended_apps_count,
)
# DEPRECATED FLOW - begin
if owner.integration_id and (
Expand Down
42 changes: 31 additions & 11 deletions tasks/notify.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,24 +210,44 @@ def run_impl_within_lock(
self.log_checkpoint(kwargs, UploadFlow.NOTIF_NO_VALID_INTEGRATION)
return {"notified": False, "notifications": None, "reason": "no_valid_bot"}
except NoConfiguredAppsAvailable as exp:
# Min wait time of 1 minute
retry_delay_seconds = max(60, get_seconds_to_next_hour())
if exp.rate_limited_count > 0:
# There's at least 1 app that we can use to communicate with GitHub,
# but this app happens to be rate limited now. We try again later.
# Min wait time of 1 minute
retry_delay_seconds = max(60, get_seconds_to_next_hour())
log.warning(
"Unable to start notifications. Retrying again later.",
extra=dict(
repoid=repoid,
commit=commitid,
apps_available=exp.apps_count,
apps_rate_limited=exp.rate_limited_count,
apps_suspended=exp.suspended_count,
countdown_seconds=retry_delay_seconds,
),
)
return self._attempt_retry(
max_retries=10,
countdown=retry_delay_seconds,
current_yaml=current_yaml,
commit=commit,
**kwargs,
)
# Maybe we have apps that are suspended. We can't communicate with github.
log.warning(
"Unable to start notifications because all ghapps available are rate limited",
"We can't find an app to communicate with GitHub. Not notifying.",
extra=dict(
repoid=repoid,
commit=commitid,
apps_available=exp.apps_count,
countdown_seconds=retry_delay_seconds,
apps_suspended=exp.suspended_count,
),
)
return self._attempt_retry(
max_retries=10,
countdown=retry_delay_seconds,
current_yaml=current_yaml,
commit=commit,
**kwargs,
)
return {
"notified": False,
"notifications": None,
"reason": "no_valid_github_app_found",
}

if current_yaml is None:
current_yaml = async_to_sync(get_current_yaml)(commit, repository_service)
Expand Down
37 changes: 35 additions & 2 deletions tasks/tests/unit/test_notify_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -809,13 +809,13 @@ def test_notify_task_no_bot(self, dbsession, mocker):
assert expected_result == res

@freeze_time("2024-04-22T11:15:00")
def test_notify_task_no_ghapp_available(self, dbsession, mocker):
def test_notify_task_no_ghapp_available_one_rate_limited(self, dbsession, mocker):
get_repo_provider_service = mocker.patch(
"tasks.notify.get_repo_provider_service"
)
mock_retry = mocker.patch.object(NotifyTask, "retry", return_value=None)
get_repo_provider_service.side_effect = NoConfiguredAppsAvailable(
apps_count=2, all_rate_limited=True
apps_count=2, rate_limited_count=1, suspended_count=1
)
commit = CommitFactory.create(
message="",
Expand All @@ -837,6 +837,39 @@ def test_notify_task_no_ghapp_available(self, dbsession, mocker):
assert res is None
mock_retry.assert_called_with(max_retries=10, countdown=45 * 60)

@freeze_time("2024-04-22T11:15:00")
def test_notify_task_no_ghapp_available_all_suspended(self, dbsession, mocker):
get_repo_provider_service = mocker.patch(
"tasks.notify.get_repo_provider_service"
)
mock_retry = mocker.patch.object(NotifyTask, "retry", return_value=None)
get_repo_provider_service.side_effect = NoConfiguredAppsAvailable(
apps_count=1, rate_limited_count=0, suspended_count=1
)
commit = CommitFactory.create(
message="",
pullid=None,
branch="test-branch-1",
commitid="649eaaf2924e92dc7fd8d370ddb857033231e67a",
repository__using_integration=True,
)
dbsession.add(commit)
dbsession.flush()
current_yaml = {"codecov": {"require_ci_to_pass": True}}
task = NotifyTask()
res = task.run_impl_within_lock(
dbsession,
repoid=commit.repoid,
commitid=commit.commitid,
current_yaml=current_yaml,
)
assert res == {
"notified": False,
"notifications": None,
"reason": "no_valid_github_app_found",
}
mock_retry.assert_not_called()

def test_submit_third_party_notifications_exception(self, mocker, dbsession):
current_yaml = {}
repository = RepositoryFactory.create()
Expand Down
Loading