diff --git a/freight/checks/github_apps.py b/freight/checks/github_apps.py index a71cd20b..04fe70ce 100644 --- a/freight/checks/github_apps.py +++ b/freight/checks/github_apps.py @@ -4,12 +4,18 @@ from flask import current_app from freight import http +from freight.config import redis +from freight.models import App from freight.exceptions import CheckFailed, CheckPending from .base import Check ERR_CHECK = "[{status}] {name}: {description} ({details_url})" ERR_MISSING_CONTEXT = "{} context was not found" +ERR_NO_CONTEXT_ON_FIRST_RUN = "No GitHub contexts found (yet)" +ERR_NO_CONTEXT = "No contexts were present on GitHub" + +FIRST_CHECK_TIMEOUT = 120 class GitHubAppsContextCheck(Check): @@ -21,7 +27,31 @@ def get_options(self): "repo": {"required": True}, } - def check(self, app, sha, config): + def _retry_on_first_run(self, condition: bool, app: App, sha: str, reason: str, + msg_pending: str, msg_failed: str): + """ + Retry the check later if the given condition is True and it's the first run + + We need to pass the condition and clean up if it's False because we have no + information about deploy ID here. We rely on the fact that only one deploy and check + can be executed for the same app at the given moment. + """ + cache_key = f"github_apps:{app.name}:{sha}:{reason}" + exists = b'1' + + if condition: + if redis.get(cache_key) == exists: + # Not the first run => probably a configuration error + redis.delete(cache_key) + raise CheckFailed(msg_failed) + else: + # First run => retry + redis.set(cache_key, exists, ex=FIRST_CHECK_TIMEOUT) + raise CheckPending(msg_pending) + else: + redis.delete(cache_key) + + def check(self, app: App, sha: str, config: dict): token = current_app.config["GITHUB_TOKEN"] if not token: raise CheckFailed("GITHUB_TOKEN is not set") @@ -44,8 +74,12 @@ def check(self, app, sha, config): resp = http.get(url, headers=headers) check_runs_dict = resp.json() - if not check_runs_dict or not check_runs_dict.get("total_count"): - raise CheckFailed("No contexts were present in GitHub") + # GitHub is sometimes slow to update commit context, so we can + # tolerate empty context list on the first run + self._retry_on_first_run(condition=(not check_runs_dict or not check_runs_dict.get("total_count")), + app=app, sha=sha, reason="empty_context", + msg_pending=ERR_NO_CONTEXT_ON_FIRST_RUN, + msg_failed=ERR_NO_CONTEXT) valid_contexts = set() for check_run in check_runs_dict.get("check_runs", []): @@ -70,5 +104,9 @@ def check(self, app, sha, config): raise CheckFailed(ERR_CHECK.format(**check_run)) contexts.remove(check_name) - if contexts: - raise CheckFailed(ERR_MISSING_CONTEXT.format(next(iter(contexts)))) + # Retry on the first iteration, since GitHub might need some time + first_context = next(iter(contexts)) if contexts else None + self._retry_on_first_run(condition=bool(contexts), + app=app, sha=sha, reason="missing_context", + msg_pending=ERR_NO_CONTEXT_ON_FIRST_RUN, + msg_failed=ERR_MISSING_CONTEXT.format(first_context)) diff --git a/tests/checks/test_github_apps.py b/tests/checks/test_github_apps.py index 5852b2d3..e8e90d81 100644 --- a/tests/checks/test_github_apps.py +++ b/tests/checks/test_github_apps.py @@ -97,6 +97,8 @@ def test_missing_required_context(self): config = {"contexts": ["other"], "repo": "getsentry/freight"} + with pytest.raises(CheckPending): + self.check.check(self.app, "abcdefg", config) with pytest.raises(CheckFailed): self.check.check(self.app, "abcdefg", config) @@ -137,6 +139,8 @@ def test_no_contexts_with_none_defined(self): config = {"repo": "getsentry/freight"} + with pytest.raises(CheckPending): + self.check.check(self.app, "abcdefg", config) with pytest.raises(CheckFailed): self.check.check(self.app, "abcdefg", config)