[IMP] runbot_merge: split status configuration into own object

Having the required statuses be a mere list of contexts has become a
bit too limiting for our needs as it doesn't allow e.g. adding new
required statuses on only some branches of a repository (e.g. only
master), nor does it allow putting checks on only branches, or only
stagings, which would be useful for overridable checks and the like,
or for checks which only make sense linked to a specific revision
range (e.g. "incremental" linting which would only check whatever's
been modified in a PR).

Split the required statuses into a separate set of objects, any of
which can be separately marked as applying only to some branches (no
branch = all branches).

Fixes #382

conftest.py

@@ -1087,7 +1087,6 @@ class Model:
         return self._env(self._model, name, self._ids, *args, **kwargs)
 
     def __setattr__(self, fieldname, value):
-        assert self._fields[fieldname]['type'] not in ('many2one', 'one2many', 'many2many')
         self._env(self._model, 'write', self._ids, {fieldname: value})
 
     def __iter__(self):

runbot_merge/models/pull_requests.py

@@ -1,8 +1,9 @@
+# coding: utf-8
+
 import ast
 import base64
 import collections
 import datetime
-import functools
 import io
 import itertools
 import json
@@ -10,7 +11,6 @@ import logging
 import os
 import pprint
 import re
-import sys
 import time
 
 from itertools import takewhile
@@ -65,9 +65,6 @@ class Project(models.Model):
              "will lead to webhook rejection. Should only use ASCII."
     )
 
-    def _create_stagings(self, commit=False):
-        pass
-
     def _check_stagings(self, commit=False):
         for staging in self.search([]).mapped('branch_ids.active_staging_id'):
             staging.check_status()
@@ -217,6 +214,20 @@ class Project(models.Model):
         """, (self.id, name))
         return bool(self.env.cr.rowcount)
 
+class StatusConfiguration(models.Model):
+    _name = 'runbot_merge.repository.status'
+    _description = "required statuses on repositories"
+    _rec_name = 'context'
+    _log_access = False
+
+    context = fields.Char(required=True)
+    repo_id = fields.Many2one('runbot_merge.repository', required=True, ondelete='cascade')
+    branch_ids = fields.Many2many('runbot_merge.branch', 'runbot_merge_repository_status_branch', 'status_id', 'branch_id')
+
+    def _for_branch(self, branch):
+        assert branch._name == 'runbot_merge.branch', \
+            f'Expected branch, got {branch}'
+        return self.filtered(lambda st: not st.branch_ids or branch in st.branch_ids)
 class Repository(models.Model):
     _name = _description = 'runbot_merge.repository'
     _order = 'sequence, id'
@@ -224,11 +235,8 @@ class Repository(models.Model):
     sequence = fields.Integer(default=50)
     name = fields.Char(required=True)
     project_id = fields.Many2one('runbot_merge.project', required=True)
-    required_statuses = fields.Char(
-        help="Comma-separated list of status contexts which must be "\
-        "`success` for a PR or staging to be valid",
-        default='legal/cla,ci/runbot'
-    )
+    status_ids = fields.One2many('runbot_merge.repository.status', 'repo_id', string="Required Statuses")
+
     branch_filter = fields.Char(default='[(1, "=", 1)]', help="Filter branches valid for this repository")
     substitutions = fields.Text(
         "label substitutions",
@@ -237,6 +245,22 @@ class Repository(models.Model):
 All substitutions are tentatively applied sequentially to the input.
 """)
 
+    @api.model
+    def create(self, vals):
+        if 'status_ids' in vals:
+            return super().create(vals)
+
+        st = vals.pop('required_statuses', 'legal/cla,ci/runbot')
+        if st:
+            vals['status_ids'] = [(0, 0, {'context': c}) for c in st.split(',')]
+        return super().create(vals)
+
+    def write(self, vals):
+        st = vals.pop('required_statuses', None)
+        if st:
+            vals['status_ids'] = [(5, 0, {})] + [(0, 0, {'context': c}) for c in st.split(',')]
+        return super().write(vals)
+
     def github(self, token_field='github_token'):
         return github.GH(self.project_id[token_field], self.name)
 
@@ -662,27 +686,27 @@ class PullRequests(models.Model):
                 pr.blocked = 'linked pr %s is not ready' % unready.display_name
                 continue
 
-    @api.depends('head', 'repository.required_statuses')
+    @api.depends('head', 'repository.status_ids')
     def _compute_statuses(self):
         Commits = self.env['runbot_merge.commit']
-        for s in self:
-            c = Commits.search([('sha', '=', s.head)])
+        for pr in self:
+            c = Commits.search([('sha', '=', pr.head)])
             if not (c and c.statuses):
-                s.status = s.statuses = False
+                pr.status = pr.statuses = False
                 continue
 
             statuses = json.loads(c.statuses)
-            s.statuses = pprint.pformat(statuses)
+            pr.statuses = pprint.pformat(statuses)
 
             st = 'success'
-            for ci in filter(None, (s.repository.required_statuses or '').split(',')):
-                v = state_(statuses, ci) or 'pending'
+            for ci in pr.repository.status_ids._for_branch(pr.target):
+                v = state_(statuses, ci.context) or 'pending'
                 if v in ('error', 'failure'):
                     st = 'failure'
                     break
                 if v == 'pending':
                     st = 'pending'
-            s.status = st
+            pr.status = st
 
     @api.depends('batch_ids.active')
     def _compute_active_batch(self):
@@ -944,7 +968,7 @@ class PullRequests(models.Model):
         # targets
         failed = self.browse(())
         for pr in self:
-            required = filter(None, (pr.repository.required_statuses or '').split(','))
+            required = pr.repository.status_ids._for_branch(pr.target).mapped('context')
 
             success = True
             for ci in required:
@@ -1438,7 +1462,7 @@ class Commit(models.Model):
     def _auto_init(self):
         res = super(Commit, self)._auto_init()
         self._cr.execute("""
-            CREATE INDEX IF NOT EXISTS runbot_merge_unique_statuses 
+            CREATE INDEX IF NOT EXISTS runbot_merge_unique_statuses
             ON runbot_merge_commit
             USING hash (sha)
         """)
@@ -1523,10 +1547,9 @@ class Stagings(models.Model):
             }
             # maps commits to the statuses they need
             required_statuses = [
-                (head, repos[repo].required_statuses.split(','))
+                (head, repos[repo].status_ids._for_branch(s.target).mapped('context'))
                 for repo, head in json.loads(s.heads).items()
                 if not repo.endswith('^')
-                if repos[repo].required_statuses
             ]
             # maps commits to their statuses
             cmap = {

runbot_merge/security/ir.model.access.csv

@@ -1,6 +1,7 @@
 id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
 access_runbot_merge_project_admin,Admin access to project,model_runbot_merge_project,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_repository_admin,Admin access to repo,model_runbot_merge_repository,runbot_merge.group_admin,1,1,1,1
+access_runbot_merge_repository_status_admin,Admin access to repo statuses,model_runbot_merge_repository_status,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_branch_admin,Admin access to branches,model_runbot_merge_branch,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_pull_requests_admin,Admin access to PR,model_runbot_merge_pull_requests,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_pull_requests_tagging_admin,Admin access to tagging,model_runbot_merge_pull_requests_tagging,runbot_merge.group_admin,1,1,1,1

runbot_merge/tests/test_basic.py

@@ -982,7 +982,7 @@ class TestNoRequiredStatus:
     def test_basic(self, env, repo, config):
         """ check that mergebot can work on a repo with no CI at all
         """
-        env['runbot_merge.repository'].search([('name', '=', repo.name)]).required_statuses = False
+        env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'0': '0'})
             repo.make_ref('heads/master', m)
@@ -1004,7 +1004,7 @@ class TestNoRequiredStatus:
         assert pr.state == 'merged'
 
     def test_updated(self, env, repo, config):
-        env['runbot_merge.repository'].search([('name', '=', repo.name)]).required_statuses = False
+        env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'0': '0'})
             repo.make_ref('heads/master', m)

runbot_merge/tests/test_by_branch.py

@@ -0,0 +1,96 @@
+import pytest
+
+from utils import Commit
+
+
+@pytest.fixture
+def repo(env, project, make_repo, users, setreviewers):
+    r = make_repo('repo')
+    project.write({
+        'repo_ids': [(0, 0, {
+            'name': r.name,
+            'status_ids': [
+                (0, 0, {'context': 'ci'}),
+                # require the lint status on master
+                (0, 0, {
+                    'context': 'lint',
+                    'branch_ids': [(4, project.branch_ids.id, False)]
+                }),
+            ]
+        })],
+    })
+    setreviewers(*project.repo_ids)
+    return r
+
+def test_status_applies(env, repo, config):
+    """ If branches are associated with a repo status, only those branch should
+    require the status on their PRs & stagings
+    """
+    with repo:
+        m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
+
+        [c] = repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change')
+        pr = repo.make_pr(target='master', title="super change", head='change')
+    pr_id = env['runbot_merge.pull_requests'].search([
+        ('repository.name', '=', repo.name),
+        ('number', '=', pr.number)
+    ])
+    assert pr_id.state == 'opened'
+
+    with repo:
+        repo.post_status(c, 'success', 'ci')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert pr_id.state == 'opened'
+
+    with repo:
+        repo.post_status(c, 'success', 'lint')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert pr_id.state == 'validated'
+
+    with repo:
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    st = env['runbot_merge.stagings'].search([])
+    assert st.state == 'pending'
+    with repo:
+        repo.post_status('staging.master', 'success', 'ci')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert st.state == 'pending'
+    with repo:
+        repo.post_status('staging.master', 'success', 'lint')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert st.state == 'success'
+
+def test_status_skipped(env, project, repo, config):
+    """ Branches not associated with a repo status should not require the status
+    on their PRs or stagings
+    """
+    # add a second branch for which the lint status doesn't apply
+    project.write({'branch_ids': [(0, 0, {'name': 'maintenance'})]})
+    with repo:
+        m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/maintenance')
+
+        [c] = repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change')
+        pr = repo.make_pr(target='maintenance', title="super change", head='change')
+    pr_id = env['runbot_merge.pull_requests'].search([
+        ('repository.name', '=', repo.name),
+        ('number', '=', pr.number)
+    ])
+    assert pr_id.state == 'opened'
+
+    with repo:
+        repo.post_status(c, 'success', 'ci')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert pr_id.state == 'validated'
+
+    with repo:
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    st = env['runbot_merge.stagings'].search([])
+    assert st.state == 'pending'
+    with repo:
+        repo.post_status('staging.maintenance', 'success', 'ci')
+    env.run_crons('runbot_merge.process_updated_commits')
+    assert st.state == 'success'

runbot_merge/views/mergebot.xml

@@ -5,13 +5,6 @@
         <field name="arch" type="xml">
             <form>
                 <sheet>
-                    <!--
-                    <div class="oe_button_box" name="button_box">
-                        <button class="oe_stat_button" name="action_see_attachments" type="object" icon="fa-book" attrs="{'invisible': ['|', ('state', '=', 'confirmed'), ('type', '=', 'routing')]}">
-                            <field string="Attachments" name="mrp_document_count" widget="statinfo"/>
-                        </button>
-                    </div>
-                    -->
                     <div class="oe_title">
                         <h1><field name="name" placeholder="Name"/></h1>
                     </div>
@@ -33,11 +26,11 @@
 
                     <separator string="Repositories"/>
                     <field name="repo_ids">
-                        <tree editable="bottom">
+                        <tree>
                             <field name="sequence" widget="handle"/>
                             <field name="name"/>
-                            <field name="required_statuses"/>
                             <field name="branch_filter"/>
+                            <field name="status_ids" widget="many2many_tags"/>
                         </tree>
                     </field>
                     <separator string="Branches"/>
@@ -53,6 +46,32 @@
         </field>
     </record>
 
+    <record id="form_repository" model="ir.ui.view">
+        <field name="name">Repository form</field>
+        <field name="model">runbot_merge.repository</field>
+        <field name="arch" type="xml">
+            <form>
+                <sheet>
+                    <div class="oe_title">
+                        <h1><field name="name"/></h1>
+                    </div>
+                    <group>
+                        <group>
+                            <field name="branch_filter"/>
+                        </group>
+                    </group>
+                    <separator string="Required Statuses"/>
+                    <field name="status_ids">
+                        <tree editable="bottom">
+                            <field name="context"/>
+                            <field name="branch_ids" widget="many2many_tags"/>
+                        </tree>
+                    </field>
+                </sheet>
+            </form>
+        </field>
+    </record>
+
     <record id="runbot_merge_action_projects" model="ir.actions.act_window">
         <field name="name">Projects</field>
         <field name="res_model">runbot_merge.project</field>