From 3cc87051ddc2cd87d28d9d7274e197aa76bd9c9e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 13 Jan 2021 12:32:24 +0100 Subject: [PATCH] [FIX] runbot_merge: avoid repeatedly warning about the same failures The mergebot has a feature to ping users when an approved PR or forward-port suffers from a CI failure, as those PRs might be somewhat unattended (so the author needs to be warned explicitly). Because the runbot can send the same failure information multiple times, the mergebot also has a *deduplication* feature, however this deduplication feature was too weak to handle the case where the PR has 2+ failures e.g. ci and linting as it only stores the last-seen failure, and there would be two different failures here. Worse, because the validation step looks at all required statuses, in that case it would send a failure ping message for each failed status *on each inbound status*: first it'd notify about the ci failure and store that, then it'd see the linting failure, check against the previous (ci), consider it a new failure, notify, and store that. Rinse and repeat every time runbot sends a ci *or* lint failure, leading to a lot of dumb and useless spam. Fix by storing the entire current failure state (a map of context: status) instead of just the last-seen status data. Note: includes a backwards-compatibility shim where we just convert a stored status into a full `{context: status}` map. This uses the "current context" because we don't have the original, but if it was a different context it's not going to match anyway (the target_url should be different) and if it was the same context then there's a chance we skip sending a redundant notification. Fixes #435 --- runbot_merge/models/pull_requests.py | 9 +++++---- runbot_merge/tests/test_basic.py | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 1e73bfb7..62319607 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1113,11 +1113,12 @@ class PullRequests(models.Model): return failed def _notify_ci_new_failure(self, ci, st): - # only sending notification if the newly failed status is different than - # the old one prev = json.loads(self.previous_failure) - if not self._statuses_equivalent(st, prev): - self.previous_failure = json.dumps(st) + if prev.get('state'): # old-style previous-failure + prev = {ci: prev} + if not any(self._statuses_equivalent(st, v) for v in prev.values()): + prev[ci] = st + self.previous_failure = json.dumps(prev) self._notify_ci_failed(ci) def _statuses_equivalent(self, a, b): diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index bba5b20b..d484e086 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -940,17 +940,24 @@ def test_ci_failure_after_review(env, repo, users, config): prx.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() - with repo: - repo.post_status(prx.head, 'failure', 'ci/runbot', target_url="https://a") - repo.post_status(prx.head, 'failure', 'legal/cla', target_url="https://b") - repo.post_status(prx.head, 'failure', 'foo/bar', target_url="https://c") - env.run_crons() + for ctx, url in [ + ('ci/runbot', 'https://a'), + ('ci/runbot', 'https://a'), + ('legal/cla', 'https://b'), + ('foo/bar', 'https://c'), + ('ci/runbot', 'https://a'), + ('legal/cla', 'https://d'), # url changes so different from the previous + ]: + with repo: + repo.post_status(prx.head, 'failure', ctx, target_url=url) + env.run_crons() assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)), + (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), + (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), ] def test_reopen_merged_pr(env, repo, config, users):