From 4bdf7e5edaf6b0bcbad5f80d2bcab03cde670d00 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 7 Feb 2020 08:22:52 +0100
Subject: [PATCH] [FIX] runbot_merge: stagings involving repos w/o required
 statuses

PRs transitioning to 'ready' had been checked and tested but turns out
I had completely forgotten to test that stagings would validate
properly therefore of course they didn't.

The issue here was I'd forgotten `''.split(',')` returns `['']` rather
than `[]`, so on an empty required_statuses the staging validator
would keep looking for a status matching the context `''` and would
never find it, keeping the staging pending until timeout. So most
likely the problem could have been resolved by just adding a condition
to

    [r.strip() for r in repomap[c.sha].required_statuses.split(',')]

but I'd already done all the rest of the reorganisation by that point,
test pass and I think it's a somewhat better logic. Therefore I'll go
with that for now.

* properly handle empty required_statuses during staging validation
* remove the final postcondition, if we're missing commits which don't
  require any statuse we should not care
* expand test to include up to merging PRs
* automatically create dummy commits when creating stagings, that way
  the relevant commits are in the database (can't hurt)

PS: an other alternative would have been to filter out or skip ahead
on commits which don't require any statuses aka cmap &
required_statuse / cmap would not even have that entry
---
 runbot_merge/models/pull_requests.py | 29 +++++++++++++++-------------
 runbot_merge/tests/test_basic.py     | 10 ++++++++--
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index 2cc671d4..ba3cfd0d 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -432,6 +432,10 @@ class Branch(models.Model):
             # $repo is the head to check, $repo^ is the head to merge
             heads[repo.name + '^'] = it['head']
             heads[repo.name] = dummy_head['sha']
+            self.env['runbot_merge.commit'].create({
+                'sha': dummy_head['sha'],
+                'statuses': '{}',
+            })
 
         # create actual staging object
         st = self.env['runbot_merge.stagings'].create({
@@ -1466,22 +1470,25 @@ class Stagings(models.Model):
             repos = {
                 repo.name: repo
                 for repo in self.env['runbot_merge.repository'].search([])
+                    .having_branch(s.target)
             }
-            repomap = {
-                head: repos[repo]
+            # maps commits to the statuses they need
+            required_statuses = [
+                (head, repos[repo].required_statuses.split(','))
                 for repo, head in json.loads(s.heads).items()
                 if not repo.endswith('^')
+            ]
+            # maps commits to their statuses
+            cmap = {
+                c.sha: json.loads(c.statuses)
+                for c in Commits.search([('sha', 'in', [h for h, _ in required_statuses])])
             }
-            commits = Commits.search([
-                ('sha', 'in', list(repomap))
-            ])
 
             update_timeout_limit = False
             st = 'success'
-            for c in commits:
-                reqs = [r.strip() for r in repomap[c.sha].required_statuses.split(',')]
-                statuses = json.loads(c.statuses)
-                for v in map(lambda n: state_(statuses, n), reqs):
+            for head, reqs in required_statuses:
+                statuses = cmap.get(head) or {}
+                for v in map(lambda n: state_(statuses, n), filter(None, reqs)):
                     if st == 'failure' or v in ('error', 'failure'):
                         st = 'failure'
                     elif v is None:
@@ -1491,10 +1498,6 @@ class Stagings(models.Model):
                         update_timeout_limit = True
                     else:
                         assert v == 'success'
-            # mark failure as soon as we find a failed status, but wait until
-            # all commits are known & not pending to mark a success
-            if st == 'success' and len(commits) < len(repomap):
-                st = 'pending'
 
             vals = {'state': st}
             if update_timeout_limit:
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 50b66a68..cabd6fe5 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -950,10 +950,16 @@ def test_no_required_statuses(env, repo, config):
         prx.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
-    assert env['runbot_merge.pull_requests'].search([
+    pr = env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
         ('number', '=', prx.number)
-    ]).state == 'ready'
+    ])
+    assert pr.state == 'ready'
+    st = pr.staging_id
+    assert st
+    env.run_crons()
+    assert st.state == 'success'
+    assert pr.state == 'merged'
 
 class TestRetry:
     @pytest.mark.xfail(reason="This may not be a good idea as it could lead to tons of rebuild spam")