From be228a5681d8ecc4c71789de9279c4f683f1dcfb Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 10 Jul 2020 10:21:43 +0200 Subject: [PATCH] [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 | 1 - runbot_merge/models/pull_requests.py | 67 ++++++++++------ runbot_merge/security/ir.model.access.csv | 1 + runbot_merge/tests/remote.py | 0 runbot_merge/tests/test_basic.py | 4 +- runbot_merge/tests/test_by_branch.py | 96 +++++++++++++++++++++++ runbot_merge/views/mergebot.xml | 37 ++++++--- 7 files changed, 172 insertions(+), 34 deletions(-) delete mode 100644 runbot_merge/tests/remote.py create mode 100644 runbot_merge/tests/test_by_branch.py diff --git a/conftest.py b/conftest.py index 388ae239..498605b3 100644 --- a/conftest.py +++ b/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): diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index fd03822b..8d38f047 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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 = { diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 87191796..cc5a4668 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/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 diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py deleted file mode 100644 index e69de29b..00000000 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 18112be6..331a2687 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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) diff --git a/runbot_merge/tests/test_by_branch.py b/runbot_merge/tests/test_by_branch.py new file mode 100644 index 00000000..f51098aa --- /dev/null +++ b/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' diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml index 1d348c4d..566d5dbf 100644 --- a/runbot_merge/views/mergebot.xml +++ b/runbot_merge/views/mergebot.xml @@ -5,13 +5,6 @@
-

@@ -33,11 +26,11 @@ - + - + @@ -53,6 +46,32 @@ + + Repository form + runbot_merge.repository + + + +
+

+
+ + + + + + + + + + + + + + + + + Projects runbot_merge.project