From 35dbfd2c1211786fbe44456108969e5472da2f37 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 31 Jan 2020 10:36:01 +0100 Subject: [PATCH] [FIX] runbot_merge: status deduplication (maybe) Despite the existing dedup' sometimes the "xxx failed on this forward-port PR" would still get multiplicated due to split builds e.g. in odoo/odoo#43935 4 such messages appear within ~5 minutes, then one more 10mn later. This is despite all of them having the same "build" (target_url) and status (failure). Since the description is the only thing that's not logged I assume that's the field which varies and makes the dedup' fail. Therefore: * add the description to the logging (when getting a status ping) * exclude the description when checking if a new status should be taken in account or ignored: the build (and thus url) should change on rebuild Hopefully fixes #281 --- forwardport/tests/test_simple.py | 3 +++ runbot_merge/controllers/__init__.py | 2 +- runbot_merge/models/pull_requests.py | 11 ++++++++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index f5e6de14..e09053b0 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -261,6 +261,9 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port with prod: prod.post_status(pr1.head, st, ctx) env.run_crons() + with prod: # should be ignored because the description doesn't matter + prod.post_status(pr1.head, 'failure', 'ci/runbot', description="HAHAHAHAHA") + env.run_crons() # check that FP did not resume & we have a ping on the PR assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ "forward port should not continue on CI failure" diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 05e34870..d17c12b5 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -207,7 +207,7 @@ def handle_pr(env, event): def handle_status(env, event): _logger.info( - 'status %(context)s:%(state)s on commit %(sha)s (%(target_url)s)', + 'status on %(sha)s %(context)s:%(state)s (%(target_url)s) [%(description)r]', event ) Commits = env['runbot_merge.commit'] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 66f6d635..316882d1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -879,10 +879,19 @@ class PullRequests(models.Model): # only sending notification if the newly failed status is different than # the old one prev = json.loads(self.previous_failure) - if st != prev: + if not self._statuses_equivalent(st, prev): self.previous_failure = json.dumps(st) self._notify_ci_failed(ci) + def _statuses_equivalent(self, a, b): + """ Check if two statuses are *equivalent* meaning the description field + is ignored (check only state and target_url). This is because the + description seems to vary even if the rest does not, and generates + unnecessary notififcations as a result + """ + return a.get('state') == b.get('state') \ + and a.get('target_url') == b.get('target_url') + def _notify_ci_failed(self, ci): # only report an issue of the PR is already approved (r+'d) if self.state == 'approved':