[ADD] runbot_merge: support for non-webhook staging validation

Add support for the ability to validate *stagings* over RPC rather
than via webhook. This may later be expanded to PRs as well.

The core motivation for this is to avoid bouncing through github which
sometimes drops the ball on statuses, and it's frustrating to have a
staging time out because GH fucked up.

Implemented via RPC, requiring both the staging itself (by id) and the
head commit being affected, as that is necessary to know what CIs are
required for that head and correctly report cross branch on the
various PRs.

Fix #881 (kinda)

runbot_merge/models/project.py

@@ -33,6 +33,7 @@ class Project(models.Model):
         ('ready', "Ready PRs over split"),
     ], default="default", required=True)
     staging_statuses = fields.Boolean(default=True)
+    staging_rpc = fields.Boolean(default=False)
 
     ci_timeout = fields.Integer(
         default=60, required=True, group_operator=None,

runbot_merge/models/pull_requests.py

@@ -18,6 +18,7 @@ import sentry_sdk
 import werkzeug
 
 from odoo import api, fields, models, tools, Command
+from odoo.exceptions import AccessError
 from odoo.osv import expression
 from odoo.tools import html_escape, Reverse
 from . import commands
@@ -1813,14 +1814,6 @@ class Stagings(models.Model):
     statuses = fields.Binary(compute='_compute_statuses')
     statuses_cache = fields.Text(default='{}', required=True)
 
-    def write(self, vals):
-        # don't allow updating the statuses_cache
-        vals.pop('statuses_cache', None)
-
-        super().write(vals)
-
-        return True
-
     @api.depends('staged_at', 'staging_end')
     def _compute_duration(self):
         for s in self:
@@ -1896,6 +1889,27 @@ class Stagings(models.Model):
         """, {'sha': c.sha, 'statuses': c.statuses, 'ids': self.ids})
         self.modified(['statuses_cache'])
 
+    def post_status(self, sha, context, status, *, target_url=None, description=None):
+        if not self.env.user.has_group('runbot_merge.status'):
+            raise AccessError("You are not allowed to post a status.")
+
+        for s in self:
+            if not s.target.project_id.staging_rpc:
+                continue
+
+            if not any(c.commit_id.sha == sha for c in s.commits):
+                raise ValueError(f"Staging {s.id} does not have the commit {sha}")
+
+            st = json.loads(s.statuses_cache)
+            st.setdefault(sha, {})[context] = {
+                'state': status,
+                'target_url': target_url,
+                'description': description,
+            }
+            s.statuses_cache = json.dumps(st)
+
+        return True
+
     @api.depends(
         "statuses_cache",
         "target",
@@ -2006,17 +2020,18 @@ class Stagings(models.Model):
             })
             return True
 
-        # single batch => the staging is an unredeemable failure
+        # single batch => the staging is an irredeemable failure
         if self.state != 'failure':
             # timed out, just mark all PRs (wheee)
             self.fail('timed out (>{} minutes)'.format(self.target.project_id.ci_timeout))
             return False
 
+        staging_statuses = json.loads(self.statuses_cache)
         # try inferring which PR failed and only mark that one
         for head in self.heads:
             required_statuses = set(head.repository_id.status_ids._for_staging(self).mapped('context'))
 
-            statuses = json.loads(head.commit_id.statuses or '{}')
+            statuses = staging_statuses.get(head.commit_id.sha, {})
             reason = next((
                 ctx for ctx, result in statuses.items()
                 if ctx in required_statuses

runbot_merge/security/security.xml

@@ -5,4 +5,7 @@
     <record model="res.groups" id="base.group_system">
         <field name="implied_ids" eval="[(4, ref('runbot_merge.group_admin'))]"/>
     </record>
+    <record model="res.groups" id="status">
+        <field name="name">Mergebot Status Sender</field>
+    </record>
 </odoo>

runbot_merge/tests/test_basic.py

@@ -24,6 +24,41 @@ def repo(env, project, make_repo, users, setreviewers):
     setreviewers(*project.repo_ids)
     return r
 
+@pytest.fixture(autouse=True, params=["statuses", "rpc"])
+def stagings(request, env, project, repo):
+    """Hook in support for validating stagings via RPC calls instead of CI
+    webhooks. Transparent for the tests as long as they send statuses to
+    symbolic refs (branch names) rather than commits, although commits *would*
+    probably be doable (look up the head for the commit, then what staging it's
+    part of)
+    """
+    if request.param == "statuses":
+            yield
+    else:
+        env['res.users'].browse([env._uid]).write({
+            "groups_id": [(4, env.ref("runbot_merge.status").id, {})]
+        })
+        project.write({
+            "staging_rpc": True,
+            "staging_statuses": False,
+        })
+        RepoType = type(repo)
+        # apparently side_effect + wraps on unbound method don't work correctly,
+        # the wrapped method does get called when returning DEFAULT but *the
+        # instance (subject) is not sent along for the ride* so the call fails.
+        post_status = RepoType.post_status
+        def _post_status(repo, ref, status, context='default', **kw):
+            if not ref.startswith(('staging.', 'heads/staging.')):
+                return post_status(repo, ref, status, context, **kw)
+
+            c = repo.commit(ref)
+            branchname = ref.removeprefix('staging.').removeprefix('heads/staging.')
+            env['runbot_merge.stagings'].search([('target.name', '=', branchname)])\
+                .post_status(c.id, context, status, **kw)
+
+        with mock.patch.object(RepoType, "post_status", _post_status):
+            yield
+
 def test_trivial_flow(env, repo, page, users, config):
     # create base branch
     with repo:
@@ -81,12 +116,10 @@ def test_trivial_flow(env, repo, page, users, config):
     assert pr_page(page, pr).cssselect('.alert-primary')
 
     with repo:
-        # get head of staging branch
+        repo.post_status('staging.master', 'success', 'ci/runbot', target_url='http://foo.com/pog')
-        staging_head = repo.commit('heads/staging.master')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(staging_head.id, 'success', 'ci/runbot', target_url='http://foo.com/pog')
-        repo.post_status(staging_head.id, 'success', 'legal/cla')
         # the should not block the merge because it's not part of the requirements
-        repo.post_status(staging_head.id, 'failure', 'ci/lint', target_url='http://ignored.com/whocares')
+        repo.post_status('staging.master', 'failure', 'ci/lint', target_url='http://ignored.com/whocares')
     # need to store this because after the crons have run the staging will
     # have succeeded and been disabled
     st = pr_id.staging_id
@@ -404,18 +437,16 @@ def test_staging_ongoing(env, repo, config):
     ])
     assert p_2.state == 'ready', "PR2 should not have been staged since there is a pending staging for master"
 
-    staging_head = repo.commit('heads/staging.master')
     with repo:
-        repo.post_status(staging_head.id, 'success', 'ci/runbot')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
-        repo.post_status(staging_head.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
     env.run_crons()
     assert pr1.state == 'merged'
     assert p_2.staging_id
 
-    staging_head = repo.commit('heads/staging.master')
     with repo:
-        repo.post_status(staging_head.id, 'success', 'ci/runbot')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
-        repo.post_status(staging_head.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
     env.run_crons()
     assert p_2.state == 'merged'
 
@@ -570,7 +601,7 @@ def test_timeout_bump_on_pending(env, repo, config):
     old_timeout = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(days=15))
     st.timeout_limit = old_timeout
     with repo:
-        repo.post_status(repo.commit('heads/staging.master').id, 'pending', 'ci/runbot')
+        repo.post_status('staging.master', 'pending', 'ci/runbot')
     env.run_crons('runbot_merge.process_updated_commits')
     assert st.timeout_limit > old_timeout
 
@@ -591,11 +622,10 @@ def test_staging_ci_failure_single(env, repo, users, config, page):
     pr_id = to_pr(env, pr)
     assert pr_id.staging_id
 
-    staging_head = repo.commit('heads/staging.master')
     with repo:
-        repo.post_status(staging_head.id, 'failure', 'a/b')
+        repo.post_status('staging.master', 'failure', 'a/b')
-        repo.post_status(staging_head.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(staging_head.id, 'failure', 'ci/runbot') # stable genius
+        repo.post_status('staging.master', 'failure', 'ci/runbot') # stable genius
     env.run_crons()
     assert pr_id.state == 'error'
 
@@ -636,8 +666,8 @@ def test_ff_failure(env, repo, config, page):
     # report staging success & run cron to merge
     staging = repo.commit('heads/staging.master')
     with repo:
-        repo.post_status(staging.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(staging.id, 'success', 'ci/runbot')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
     env.run_crons()
 
     assert st.reason == 'update is not a fast forward'
@@ -933,8 +963,8 @@ def test_forward_port(env, repo, config):
     st = repo.commit('staging.master')
 
     with repo:
-        repo.post_status(st.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(st.id, 'success', 'ci/runbot')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
     env.run_crons()
 
     h = repo.commit('master')
@@ -1158,8 +1188,8 @@ class TestRetry:
         ]).staging_id
 
         staging_head = repo.commit('heads/staging.master')
-        repo.post_status(staging_head.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(staging_head.id, 'failure', 'ci/runbot')
+        repo.post_status('staging.master', 'failure', 'ci/runbot')
         env.run_crons()
         pr = env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -1178,8 +1208,8 @@ class TestRetry:
 
         staging_head2 = repo.commit('heads/staging.master')
         assert staging_head2 != staging_head
-        repo.post_status(staging_head2.id, 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'legal/cla')
-        repo.post_status(staging_head2.id, 'success', 'ci/runbot')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
         env.run_crons()
         assert pr.state == 'merged'
 
@@ -1202,8 +1232,8 @@ class TestRetry:
 
         staging_head = repo.commit('heads/staging.master')
         with repo:
-            repo.post_status(staging_head.id, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status(staging_head.id, 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure', 'ci/runbot')
         env.run_crons()
         assert env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -1221,8 +1251,8 @@ class TestRetry:
         staging_head2 = repo.commit('heads/staging.master')
         assert staging_head2 != staging_head
         with repo:
-            repo.post_status(staging_head2.id, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status(staging_head2.id, 'success', 'ci/runbot')
+            repo.post_status('staging.master', 'success', 'ci/runbot')
         env.run_crons()
         assert env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -1297,8 +1327,8 @@ class TestRetry:
 
         staging_head = repo.commit('heads/staging.master')
         with repo:
-            repo.post_status(staging_head.id, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status(staging_head.id, 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure', 'ci/runbot')
         env.run_crons()
         pr = env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -1357,8 +1387,8 @@ class TestMergeMethod:
             "dummy commit aside, the previous master's tip should be the sole parent of the staging commit"
 
         with repo:
-            repo.post_status(staging.id, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status(staging.id, 'success', 'ci/runbot')
+            repo.post_status('staging.master', 'success', 'ci/runbot')
         env.run_crons()
         pr = env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -2367,10 +2397,9 @@ class TestPRUpdate(object):
         assert pr.state == 'ready'
         assert pr.staging_id
 
-        h = repo.commit('heads/staging.master').id
         with repo:
-            repo.post_status(h, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
-            repo.post_status(h, 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure', 'ci/runbot')
         env.run_crons()
         assert not pr.staging_id
         assert pr.state == 'error'
@@ -3052,19 +3081,17 @@ class TestBatching(object):
         assert len(sp) == 1
 
         # This is the failing PR!
-        h = repo.commit('heads/staging.master').id
         with repo:
-            repo.post_status(h, 'failure', 'ci/runbot')
+            repo.post_status('staging.master', 'failure', 'ci/runbot')
-            repo.post_status(h, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
         env.run_crons()
         assert pr1.state == 'error'
 
         assert pr2.staging_id
 
-        h = repo.commit('heads/staging.master').id
         with repo:
-            repo.post_status(h, 'success', 'ci/runbot')
+            repo.post_status('staging.master', 'success', 'ci/runbot')
-            repo.post_status(h, 'success', 'legal/cla')
+            repo.post_status('staging.master', 'success', 'legal/cla')
         env.run_crons('runbot_merge.process_updated_commits', 'runbot_merge.merge_cron', 'runbot_merge.staging_cron')
         assert pr2.state == 'merged'
 

runbot_merge/views/runbot_merge_project.xml

@@ -30,6 +30,16 @@
                             <field name="github_email" readonly="0"
                                    help="Identity when creating new commits, defaults to public email, falls back to primary email."/>
                             <field name="secret"/>
+                            <span attrs="{'invisible': [
+                                '|',
+                                    ('staging_statuses', '=', False),
+                                    ('staging_rpc', '=', False),
+                            ]}" class="alert alert-warning" role="alert">
+                                Avoid overlaps between GH and RPC as the older
+                                GH statuses may overwrite more recent RPC statuses.
+                            </span>
+                            <field name="staging_statuses" string="Validate via GH statuses"/>
+                            <field name="staging_rpc" string="Validate via direct RPC"/>
                         </group>
                         <group>
                             <field name="staging_enabled" widget="boolean_toggle"/>