From 2009177adae5b8aec600c0a6d9bf916666cf3504 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 8 Jun 2023 08:19:43 +0200 Subject: [PATCH] [IMP] *: allow disabling staging on branch, remove fp target flag - currently disabling staging only works globally, allow disabling on a single branch - use a toggle - remove a pair of tests which work specifically with `fp_target`, can't work with `active` (probably) - cleanup search of possible and active stagings, add relevant indexes and use direct search of relevant branches instead of looking up from the project - also use toggle button for `active` on branches - shitty workaround for upgrading DB: apparently mail really wants to have a `user_id` to do some weird thing, so need to re-add it after resetting everything Fixes #727 --- forwardport/data/views.xml | 6 -- forwardport/models/project.py | 15 +--- forwardport/tests/test_conflicts.py | 2 +- forwardport/tests/test_limit.py | 77 +-------------------- forwardport/tests/test_updates.py | 12 ++-- forwardport/tests/test_weird.py | 16 ++--- mergebot_test_utils/utils.py | 6 +- runbot_merge/models/project.py | 31 +++++---- runbot_merge/models/pull_requests.py | 8 ++- runbot_merge/tests/test_staging.py | 41 +++++++++++ runbot_merge/views/res_partner.xml | 1 + runbot_merge/views/runbot_merge_project.xml | 3 +- 12 files changed, 88 insertions(+), 130 deletions(-) create mode 100644 runbot_merge/tests/test_staging.py diff --git a/forwardport/data/views.xml b/forwardport/data/views.xml index 46a8b48a..c25bb311 100644 --- a/forwardport/data/views.xml +++ b/forwardport/data/views.xml @@ -152,12 +152,6 @@ help="Repository where forward port branches will be created" /> - - - - diff --git a/forwardport/models/project.py b/forwardport/models/project.py index f59bdbbd..fcf7c645 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -210,17 +210,6 @@ class Repository(models.Model): _inherit = 'runbot_merge.repository' fp_remote_target = fields.Char(help="where FP branches get pushed") -class Branch(models.Model): - _inherit = 'runbot_merge.branch' - - fp_target = fields.Boolean(default=True) - fp_enabled = fields.Boolean(compute='_compute_fp_enabled') - - @api.depends('active', 'fp_target') - def _compute_fp_enabled(self): - for b in self: - b.fp_enabled = b.active and b.fp_target - class PullRequests(models.Model): _inherit = 'runbot_merge.pull_requests' @@ -438,7 +427,7 @@ class PullRequests(models.Model): ping = False msg = "Forward-port disabled." self.limit_id = limit_id - elif not limit_id.fp_enabled: + elif not limit_id.active: msg = "branch %r is disabled, it can't be used as a forward port target." % limit_id.name else: ping = False @@ -550,7 +539,7 @@ class PullRequests(models.Model): return next(( branch for branch in branches[from_+1:to_+1] - if branch.fp_enabled + if branch.active ), None) def _commits_lazy(self): diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 4e6dd084..3cda8430 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -16,7 +16,7 @@ def test_conflict(env, config, make_repo, users): project = env['runbot_merge.project'].search([]) project.write({ 'branch_ids': [ - (0, 0, {'name': 'd', 'sequence': 40, 'fp_target': True}) + (0, 0, {'name': 'd', 'sequence': 40}) ] }) diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 8310619d..fca04111 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -50,31 +50,6 @@ def test_configure(env, config, make_repo): assert prs[2].number == originals[2] -def test_self_disabled(env, config, make_repo): - """ Allow setting target as limit even if it's disabled - """ - prod, other = make_basic(env, config, make_repo) - bot_name = env['runbot_merge.project'].search([]).fp_github_name - branch_a = env['runbot_merge.branch'].search([('name', '=', 'a')]) - branch_a.fp_target = False - with prod: - [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch') - pr = prod.make_pr(target='a', head='mybranch') - prod.post_status(c, 'success', 'legal/cla') - prod.post_status(c, 'success', 'ci/runbot') - pr.post_comment('hansen r+\n%s up to a' % bot_name, config['role_reviewer']['token']) - env.run_crons() - pr_id = env['runbot_merge.pull_requests'].search([('number', '=', pr.number)]) - assert pr_id.limit_id == branch_a - - with prod: - prod.post_status('staging.a', 'success', 'legal/cla') - prod.post_status('staging.a', 'success', 'ci/runbot') - - assert env['runbot_merge.pull_requests'].search([]) == pr_id,\ - "should not have created a forward port" - - def test_ignore(env, config, make_repo): """ Provide an "ignore" command which is equivalent to setting the limit to target @@ -100,17 +75,12 @@ def test_ignore(env, config, make_repo): "should not have created a forward port" -@pytest.mark.parametrize('enabled', ['active', 'fp_target']) -def test_disable(env, config, make_repo, users, enabled): +def test_disable(env, config, make_repo, users): """ Checks behaviour if the limit target is disabled: * disable target while FP is ongoing -> skip over (and stop there so no FP) * forward-port over a disabled branch * request a disabled target as limit - - Disabling (with respect to forward ports) can be performed by marking the - branch as !active (which also affects mergebot operations), or as - !fp_target (won't be forward-ported to). """ prod, other = make_basic(env, config, make_repo) project = env['runbot_merge.project'].search([]) @@ -133,7 +103,7 @@ def test_disable(env, config, make_repo, users, enabled): prod.post_status('staging.a', 'success', 'legal/cla') prod.post_status('staging.a', 'success', 'ci/runbot') # disable branch b - env['runbot_merge.branch'].search([('name', '=', 'b')]).write({enabled: False}) + env['runbot_merge.branch'].search([('name', '=', 'b')]).active = False env.run_crons() # should have created a single PR (to branch c, for pr 1) @@ -168,49 +138,6 @@ def test_disable(env, config, make_repo, users, enabled): seen(env, pr, users), } - -def test_default_disabled(env, config, make_repo, users): - """ If the default limit is disabled, it should still be the default - limit but the ping message should be set on the actual last FP (to the - last non-deactivated target) - """ - prod, other = make_basic(env, config, make_repo) - branch_c = env['runbot_merge.branch'].search([('name', '=', 'c')]) - branch_c.fp_target = False - - with prod: - [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/branch0') - pr = prod.make_pr(target='a', head='branch0') - prod.post_status(c, 'success', 'legal/cla') - prod.post_status(c, 'success', 'ci/runbot') - pr.post_comment('hansen r+', config['role_reviewer']['token']) - env.run_crons() - - assert env['runbot_merge.pull_requests'].search([]).limit_id == branch_c - - with prod: - prod.post_status('staging.a', 'success', 'legal/cla') - prod.post_status('staging.a', 'success', 'ci/runbot') - env.run_crons() - - p1, p2 = env['runbot_merge.pull_requests'].search([], order='number') - assert p1.number == pr.number - pr2 = prod.get_pr(p2.number) - - cs = pr2.comments - assert len(cs) == 2 - assert pr2.comments == [ - seen(env, pr2, users), - (users['user'], """\ -@%(user)s @%(reviewer)s this PR targets b and is the last of the forward-port chain. - -To merge the full chain, say -> @%(user)s r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""" % users) - ] - def test_limit_after_merge(env, config, make_repo, users): """ If attempting to set a limit () on a PR which is merged (already forward-ported or not), or is a forward-port PR, fwbot should diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 99471cd7..384df08f 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -151,9 +151,7 @@ def test_update_merged(env, make_repo, config, users): with prod: prod.make_ref('heads/d', prod.commit('c').id) env['runbot_merge.project'].search([]).write({ - 'branch_ids': [(0, 0, { - 'name': 'd', 'sequence': 40, 'fp_target': True, - })] + 'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})] }) with prod: @@ -251,10 +249,10 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'master', 'sequence': 0, 'fp_target': True}), - (0, 0, {'name': 'v3', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'v2', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'v1', 'sequence': 3, 'fp_target': True}), + (0, 0, {'name': 'master', 'sequence': 0}), + (0, 0, {'name': 'v3', 'sequence': 1}), + (0, 0, {'name': 'v2', 'sequence': 2}), + (0, 0, {'name': 'v1', 'sequence': 3}), ], 'repo_ids': [ (0, 0, { diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index ab077c05..4ef56feb 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -26,9 +26,9 @@ def make_basic(env, config, make_repo, *, fp_token, fp_remote): 'github_prefix': 'hansen', 'fp_github_token': fp_token and config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 2}), + (0, 0, {'name': 'b', 'sequence': 1}), + (0, 0, {'name': 'c', 'sequence': 0}), ], }) @@ -263,9 +263,9 @@ class TestNotAllBranches: 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 2}), + (0, 0, {'name': 'b', 'sequence': 1}), + (0, 0, {'name': 'c', 'sequence': 0}), ] }) repo_a = env['runbot_merge.repository'].create({ @@ -514,7 +514,7 @@ def test_new_intermediate_branch(env, config, make_repo): env.run_crons() project.write({ 'branch_ids': [ - (0, False, {'name': 'new', 'sequence': 1, 'fp_target': True}), + (0, False, {'name': 'new', 'sequence': 1}), ] }) env.run_crons() @@ -748,7 +748,7 @@ def test_retarget_after_freeze(env, config, make_repo, users): project.write({ 'branch_ids': [ (1, branch_c.id, {'sequence': 1}), - (0, 0, {'name': 'bprime', 'sequence': 2, 'fp_target': True}), + (0, 0, {'name': 'bprime', 'sequence': 2}), (1, branch_b.id, {'sequence': 3}), (1, branch_a.id, {'sequence': 4}), ] diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 2df8995d..a23d6726 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -74,9 +74,9 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje 'github_prefix': 'hansen', 'fp_github_token': config['github']['token'], 'branch_ids': [ - (0, 0, {'name': 'a', 'sequence': 100, 'fp_target': True}), - (0, 0, {'name': 'b', 'sequence': 80, 'fp_target': True}), - (0, 0, {'name': 'c', 'sequence': 60, 'fp_target': True}), + (0, 0, {'name': 'a', 'sequence': 100}), + (0, 0, {'name': 'b', 'sequence': 80}), + (0, 0, {'name': 'c', 'sequence': 60}), ], }) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index fb46c33f..883c91c6 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -46,10 +46,11 @@ class Project(models.Model): freeze_reminder = fields.Text() def _check_stagings(self, commit=False): - for branch in self.search([]).mapped('branch_ids').filtered('active'): + # check branches with an active staging + for branch in self.env['runbot_merge.branch']\ + .with_context(active_test=False)\ + .search([('active_staging_id', '!=', False)]): staging = branch.active_staging_id - if not staging: - continue try: with self.env.cr.savepoint(): staging.check_status() @@ -61,16 +62,20 @@ class Project(models.Model): self.env.cr.commit() def _create_stagings(self, commit=False): - for branch in self.search([]).mapped('branch_ids').filtered('active'): - if not branch.active_staging_id: - try: - with self.env.cr.savepoint(): - branch.try_staging() - except Exception: - _logger.exception("Failed to create staging for branch %r", branch.name) - else: - if commit: - self.env.cr.commit() + # look up branches which can be staged on and have no active staging + for branch in self.env['runbot_merge.branch'].search([ + ('active_staging_id', '=', False), + ('active', '=', True), + ('staging_enabled', '=', True), + ]): + try: + with self.env.cr.savepoint(): + branch.try_staging() + except Exception: + _logger.exception("Failed to create staging for branch %r", branch.name) + else: + if commit: + self.env.cr.commit() def _find_commands(self, comment): return re.findall( diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8be1070b..47ea4001 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -67,7 +67,7 @@ class Repository(models.Model): sequence = fields.Integer(default=50, group_operator=None) name = fields.Char(required=True) - project_id = fields.Many2one('runbot_merge.project', required=True) + project_id = fields.Many2one('runbot_merge.project', required=True, index=True) status_ids = fields.One2many('runbot_merge.repository.status', 'repo_id', string="Required Statuses") group_id = fields.Many2one('res.groups', default=lambda self: self.env.ref('base.group_user')) @@ -235,10 +235,10 @@ class Branch(models.Model): _order = 'sequence, name' name = fields.Char(required=True) - project_id = fields.Many2one('runbot_merge.project', required=True) + project_id = fields.Many2one('runbot_merge.project', required=True, index=True) active_staging_id = fields.Many2one( - 'runbot_merge.stagings', compute='_compute_active_staging', store=True, + 'runbot_merge.stagings', compute='_compute_active_staging', store=True, index=True, help="Currently running staging for the branch." ) staging_ids = fields.One2many('runbot_merge.stagings', 'target') @@ -252,6 +252,8 @@ class Branch(models.Model): active = fields.Boolean(default=True) sequence = fields.Integer(group_operator=None) + staging_enabled = fields.Boolean(default=True) + def _auto_init(self): res = super(Branch, self)._auto_init() tools.create_unique_index( diff --git a/runbot_merge/tests/test_staging.py b/runbot_merge/tests/test_staging.py new file mode 100644 index 00000000..c594cc76 --- /dev/null +++ b/runbot_merge/tests/test_staging.py @@ -0,0 +1,41 @@ +import pytest + +from utils import Commit, to_pr + + +@pytest.fixture +def repo(env, project, make_repo, users, setreviewers): + r = make_repo('repo') + project.write({'repo_ids': [(0, 0, { + 'name': r.name, + 'group_id': False, + 'required_statuses': 'ci' + })]}) + setreviewers(*project.repo_ids) + return r + +def test_staging_disabled_branch(env, project, repo, config): + """Check that it's possible to disable staging on a specific branch + """ + project.branch_ids = [(0, 0, { + 'name': 'other', + 'staging_enabled': False, + })] + with repo: + [master_commit] = repo.make_commits(None, Commit("master", tree={'a': '1'}), ref="heads/master") + [c1] = repo.make_commits(master_commit, Commit("thing", tree={'a': '2'}), ref='heads/master-thing') + master_pr = repo.make_pr(title="whatever", target="master", head="master-thing") + master_pr.post_comment("hansen r+", config['role_reviewer']['token']) + repo.post_status(c1, 'success', 'ci') + + [other_commit] = repo.make_commits(None, Commit("other", tree={'b': '1'}), ref='heads/other') + [c2] = repo.make_commits(other_commit, Commit("thing", tree={'b': '2'}), ref='heads/other-thing') + other_pr = repo.make_pr(title="whatever", target="other", head="other-thing") + other_pr.post_comment("hansen r+", config['role_reviewer']['token']) + repo.post_status(c2, 'success', 'ci') + env.run_crons() + + assert to_pr(env, master_pr).staging_id, \ + "master is allowed to stage, should be staged" + assert not to_pr(env, other_pr).staging_id, \ + "other is *not* allowed to stage, should not be staged" diff --git a/runbot_merge/views/res_partner.xml b/runbot_merge/views/res_partner.xml index fde5f7e1..507e5a32 100644 --- a/runbot_merge/views/res_partner.xml +++ b/runbot_merge/views/res_partner.xml @@ -25,6 +25,7 @@ + diff --git a/runbot_merge/views/runbot_merge_project.xml b/runbot_merge/views/runbot_merge_project.xml index 2f670ed3..544f9cf9 100644 --- a/runbot_merge/views/runbot_merge_project.xml +++ b/runbot_merge/views/runbot_merge_project.xml @@ -56,7 +56,8 @@ - + +