From 8e8a238a66fd4c9ebe16df0343cc8a0a87beef9e Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 2 Dec 2021 11:36:10 +0100 Subject: [PATCH 1/5] [FIX] forwarport: mark branches as fp-targets by default otherwise the freeze wizard gets very confused --- forwardport/models/project.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index a1d939a5..49f3e202 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -176,7 +176,7 @@ class Branch(models.Model): _inherit = 'runbot_merge.branch' fp_sequence = fields.Integer(default=50) - fp_target = fields.Boolean(default=False) + fp_target = fields.Boolean(default=True) fp_enabled = fields.Boolean(compute='_compute_fp_enabled') @api.depends('active', 'fp_target') From 1add3d4854718db3f3c663b6492179b4f4dbbe4a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 7 Feb 2022 13:23:41 +0100 Subject: [PATCH 2/5] [FIX] runbot_merge: freeze being triggered upon reopening the wizard The freeze wizard was implemented using a single action to open and validate the dialog. This was a mistake, as it means if there are no errors left (e.g. all the PRs being waited for are now validated) trying to view the freeze wizard will immediately validate it and commit the freeze, which is unexpected, surprising, and unsafe e.g. - open wizard - add freeze prs - add a required pr or two - close and go do something else - be told that more PRs need to be waited for - reopen wizard - oops freeze is done So split the "open action" part of `action_freeze` into opening the action and performing the freeze. The "freeze" / "view freeze" button on the project only activates the latter, and the actual freeze operation is only triggered from the wizard's "Freeze" button. Part of #559. --- runbot_merge/models/project.py | 2 +- .../models/project_freeze/__init__.py | 21 +++++++++++-------- runbot_merge/tests/test_multirepo.py | 9 ++++++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index dc83b8f4..77c778e2 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -117,4 +117,4 @@ class Project(models.Model): 'branch_name': self._next_freeze(), 'release_pr_ids': [(0, 0, {'repository_id': repo.id}) for repo in self.repo_ids] }) - return w.action_freeze() + return w.action_open() diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index f795261d..92bc6c65 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -56,21 +56,24 @@ class FreezeWizard(models.Model): return {'type': 'ir.actions.act_window_close'} + def action_open(self): + return { + 'type': 'ir.actions.act_window', + 'target': 'new', + 'name': f'Freeze project {self.project_id.name}', + 'view_mode': 'form', + 'res_model': self._name, + 'res_id': self.id, + } + def action_freeze(self): """ Attempts to perform the freeze. """ - project_id = self.project_id # if there are still errors, reopen the wizard if self.errors: - return { - 'type': 'ir.actions.act_window', - 'target': 'new', - 'name': f'Freeze project {project_id.name}', - 'view_mode': 'form', - 'res_model': self._name, - 'res_id': self.id, - } + return self.action_open() + project_id = self.project_id # need to create the new branch, but at the same time resequence # everything so the new branch is the second one, just after the branch # it "forks" diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index f2aaf471..bfb4f316 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1152,6 +1152,7 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): w = project.action_prepare_freeze() w2 = project.action_prepare_freeze() assert w == w2, "each project should only have one freeze wizard active at a time" + assert w['res_model'] == 'runbot_merge.project.freeze' w_id = env[w['res_model']].browse([w['res_id']]) assert w_id.branch_name == '1.1', "check that the forking incremented the minor by 1" @@ -1188,6 +1189,14 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): assert not w_id.errors + # assume the wizard is closed, re-open it + w = project.action_prepare_freeze() + assert w['res_model'] == 'runbot_merge.project.freeze' + assert w['res_id'] == w_id.id, "check that we're still getting the old wizard" + w_id = env[w['res_model']].browse([w['res_id']]) + assert w_id.exists() + + # actually perform the freeze r = w_id.action_freeze() # check that the wizard was deleted assert not w_id.exists() From e2887a7473147d490d78c9aaec9e43b8ee5f9819 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 7 Feb 2022 15:15:13 +0100 Subject: [PATCH 3/5] [IMP] runbot_merge: allow only freezing a subset of a project - add flag to not select repos for freezing - allow removing more repositories from the wizard - when performing the freeze, only create branches for the selected repos --- runbot_merge/__manifest__.py | 2 +- runbot_merge/models/project.py | 6 +- .../models/project_freeze/__init__.py | 14 ++-- runbot_merge/models/project_freeze/views.xml | 13 +++- runbot_merge/security/ir.model.access.csv | 2 +- runbot_merge/tests/test_multirepo.py | 72 +++++++++++++++++++ 6 files changed, 101 insertions(+), 8 deletions(-) diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index e44f1830..8375a3d8 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -9,9 +9,9 @@ 'data/merge_cron.xml', 'views/res_partner.xml', 'views/runbot_merge_project.xml', - 'models/project_freeze/views.xml', 'views/mergebot.xml', 'views/templates.xml', + 'models/project_freeze/views.xml', ], 'post_load': 'enable_sentry', 'pre_init_hook': '_check_citext', diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index 77c778e2..f3d27025 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -115,6 +115,10 @@ class Project(models.Model): w = Freeze.search([('project_id', '=', self.id)]) or Freeze.create({ 'project_id': self.id, 'branch_name': self._next_freeze(), - 'release_pr_ids': [(0, 0, {'repository_id': repo.id}) for repo in self.repo_ids] + 'release_pr_ids': [ + (0, 0, {'repository_id': repo.id}) + for repo in self.repo_ids + if repo.freeze + ] }) return w.action_open() diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index 92bc6c65..39941d28 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -97,7 +97,8 @@ class FreezeWizard(models.Model): # create new branch on every repository errors = [] repository = None - for repository in project_id.repo_ids: + for rel in self.release_pr_ids: + repository = rel.repository_id gh = repository.github() # annoyance: can't directly alias a ref to an other ref, need to # resolve the "old" branch explicitely @@ -117,11 +118,11 @@ class FreezeWizard(models.Model): # if an error occurred during creation, try to clean up then raise error if errors: - for r in project_id.repo_ids: - if r == repository: + for r in self.release_pr_ids: + if r.repository_id == repository: break - deletion = r.github().delete(f'git/refs/heads/{self.branch_name}') + deletion = r.repository_id.github().delete(f'git/refs/heads/{self.branch_name}') if not deletion.ok: errors.append(f"Consequently unable to delete branch {self.branch_name} of repository {r.name}.") time.sleep(1) @@ -166,6 +167,11 @@ class ReleasePullRequest(models.Model): return super().write(vals) +class RepositoryFreeze(models.Model): + _inherit = 'runbot_merge.repository' + freeze = fields.Boolean(required=True, default=True, + help="Freeze this repository by default") + @enum.unique class Colors(enum.IntEnum): No = 0 diff --git a/runbot_merge/models/project_freeze/views.xml b/runbot_merge/models/project_freeze/views.xml index 255be83d..87b1baca 100644 --- a/runbot_merge/models/project_freeze/views.xml +++ b/runbot_merge/models/project_freeze/views.xml @@ -25,7 +25,7 @@ - + @@ -49,4 +49,15 @@ + + + Add freeze field to repo form + runbot_merge.repository + + + + + + + diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 5815aed8..4bf9e0d2 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -1,7 +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_project_freeze,Admin access to freeze wizard,model_runbot_merge_project_freeze,runbot_merge.group_admin,1,1,0,0 -access_runbot_merge_project_freeze_prs,Admin access to freeze wizard prs,model_runbot_merge_project_freeze_prs,runbot_merge.group_admin,1,1,0,0 +access_runbot_merge_project_freeze_prs,Admin access to freeze wizard prs,model_runbot_merge_project_freeze_prs,runbot_merge.group_admin,1,1,0,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 diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index bfb4f316..7b71d2f6 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1234,3 +1234,75 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): assert c_c.message.startswith('Release 1.1 (C)') assert repo_c.read_tree(c_c) == {'f': '2', 'version': ''} assert repo_c.commit(c_c.parents[0]).parents[0] == masters[2] + + +def test_freeze_subset(env, project, repo_a, repo_b, repo_c, users, config): + """It should be possible to only freeze a subset of a project when e.g. one + of the repository is managed differently than the rest and has + non-synchronous releases. + + - it should be possible to mark repositories as non-freezed (just opted out + of the entire thing), in which case no freeze PRs should be asked of them + - it should be possible to remove repositories from the freeze wizard + - repositories which are not in the freeze wizard should just not be frozen + + To do things correctly that should probably match with the branch filters + and stuff, but that's a configuration concern. + """ + # have a project with 3 repos, and two branches (1.0 and master) + project.branch_ids = [ + (1, project.branch_ids.id, {'sequence': 1}), + (0, 0, {'name': '1.0', 'sequence': 2}), + ] + + masters = [] + for r in [repo_a, repo_b, repo_c]: + with r: + [root, _] = r.make_commits( + None, + Commit('base', tree={'version': '', 'f': '0'}), + Commit('release 1.0', tree={'version': '1.0'} if r is repo_a else None), + ref='heads/1.0' + ) + masters.extend(r.make_commits(root, Commit('other', tree={'f': '1'}), ref='heads/master')) + + with repo_a: + repo_a.make_commits( + masters[0], + Commit('Release 1.1', tree={'version': '1.1'}), + ref='heads/release-1.1' + ) + pr_rel_a = repo_a.make_pr(target='master', head='release-1.1') + + # the third repository we opt out of freezing + project.repo_ids.filtered(lambda r: r.name == repo_c.name).freeze = False + env.run_crons() # process the PRs + + # open the freeze wizard + w = project.action_prepare_freeze() + w_id = env[w['res_model']].browse([w['res_id']]) + # check that there are only rels for repos A and B + assert w_id.mapped('release_pr_ids.repository_id.name') == [repo_a.name, repo_b.name] + # remove B from the set + b_id = w_id.release_pr_ids.filtered(lambda r: r.repository_id.name == repo_b.name) + w_id.write({'release_pr_ids': [(3, b_id.id, 0)]}) + assert len(w_id.release_pr_ids) == 1 + # set lone release PR + w_id.release_pr_ids.pr_id = to_pr(env, pr_rel_a).id + assert not w_id.errors + + w_id.action_freeze() + assert not w_id.exists() + + assert repo_a.commit('1.1'), "should have created branch in repo A" + try: + repo_b.commit('1.1') + pytest.fail("should *not* have created branch in repo B") + except AssertionError: + ... + try: + repo_c.commit('1.1') + pytest.fail("should *not* have created branch in repo C") + except AssertionError: + ... + # can't stage because we (wilfully) don't have branches 1.1 in repos B and C From e05cc77a5795395d9915fdb1c994e9f0cadf906d Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 8 Feb 2022 10:11:57 +0100 Subject: [PATCH 4/5] [FIX] forwardport: don't forwardport freeze PRs The freeze wizard has support for merging freeze / release PRs on each of the newly created branches. But since this would be done by, well, merging, those PRs would get forward-ported to master, and would have to be closed there. This creates additional work for the freeze master, and noise / parasitic PRs. Obviously it's possible for the freeze master to set some nonsense `up to` (nonsense because the "real" limit doesn't exist yet at that point), but really it never makes any sense to forward port release PRs, so the wizard should do it. --- forwardport/models/forwardport.py | 2 +- forwardport/models/project_freeze.py | 10 +++++- forwardport/tests/test_weird.py | 50 ++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 9440c389..e8eca387 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -34,7 +34,7 @@ class Queue: def _search_domain(self): return [] -class BatchQueue(models.Model, Queue): +class ForwardPortTasks(models.Model, Queue): _name = 'forwardport.batches' _description = 'batches which got merged and are candidates for forward-porting' diff --git a/forwardport/models/project_freeze.py b/forwardport/models/project_freeze.py index 8168e14a..ddd9f222 100644 --- a/forwardport/models/project_freeze.py +++ b/forwardport/models/project_freeze.py @@ -21,4 +21,12 @@ class FreezeWizard(models.Model): self.env.ref('forwardport.port_forward').active = True return r - + def action_freeze(self): + # have to store wizard content as it's removed during freeze + project = self.project_id + branches_before = project.branch_ids + prs = self.mapped('release_pr_ids.pr_id') + r = super().action_freeze() + new_branch = project.branch_ids - branches_before + prs.write({'limit_id': new_branch.id}) + return r diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 57915f4b..31c9e777 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -24,9 +24,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', 'fp_sequence': 2, 'fp_target': True}), - (0, 0, {'name': 'b', 'fp_sequence': 1, 'fp_target': True}), - (0, 0, {'name': 'c', 'fp_sequence': 0, 'fp_target': True}), + (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}), ], }) @@ -683,3 +683,47 @@ def test_approve_draft(env, config, make_repo, users): pr.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() assert pr_id.state == 'approved' + +def test_freeze(env, config, make_repo, users): + """Freeze: + + - should not forward-port the freeze PRs themselves + """ + project, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True) + # branches here are "a" (older), "b", and "c" (master) + with prod: + [root, _] = prod.make_commits( + None, + Commit('base', tree={'version': '', 'f': '0'}), + Commit('release 1.0', tree={'version': '1.0'}), + ref='heads/b' + ) + prod.make_commits(root, Commit('other', tree={'f': '1'}), ref='heads/c') + with prod: + prod.make_commits( + 'c', + Commit('Release 1.1', tree={'version': '1.1'}), + ref='heads/release-1.1' + ) + release = prod.make_pr(target='c', head='release-1.1') + env.run_crons() + + w = project.action_prepare_freeze() + assert w['res_model'] == 'runbot_merge.project.freeze' + w_id = env[w['res_model']].browse([w['res_id']]) + assert w_id.release_pr_ids.repository_id.name == prod.name + release_id = to_pr(env, release) + w_id.release_pr_ids.pr_id = release_id.id + + assert not w_id.errors + w_id.action_freeze() + env.run_crons() # stage freeze PRs + with prod: + prod.post_status('staging.post-b', 'success', 'ci/runbot') + prod.post_status('staging.post-b', 'success', 'legal/cla') + env.run_crons() + + assert release_id.state == 'merged' + assert not env['runbot_merge.pull_requests'].search([ + ('state', '!=', 'merged') + ]), "the release PRs should not be forward-ported" From de70bd6f8371a23652f6b2317165c1720b9469c5 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 8 Feb 2022 11:06:34 +0100 Subject: [PATCH 5/5] [IMP] runbot_merge: show PR titles in freeze wizard Currently limited to release/freeze PRs: it can be difficult to be sure the right PR was selected then, and a mistake there seems more impactful than in the PRs being waited for? Note: adds a test to make sure I don't break the check that all release PRs must have the same label (be linked). This was already safe, and in a way this PR adds convenience but not really safety, but better sure than sorry. --- runbot_merge/models/project_freeze/__init__.py | 2 +- runbot_merge/models/project_freeze/views.xml | 3 ++- runbot_merge/models/pull_requests.py | 18 +++++++++++++----- runbot_merge/tests/test_multirepo.py | 14 ++++++++++---- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index 39941d28..07c26d35 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -41,7 +41,7 @@ class FreezeWizard(models.Model): labels = set(self.mapped('release_pr_ids.pr_id.label')) if len(labels) != 1: - errors.append("* All release PRs must have the same label, found %r." % ', '.join(labels)) + errors.append("* All release PRs must have the same label, found %r." % ', '.join(sorted(labels))) unready = sum(p.state not in ('closed', 'merged') for p in self.required_pr_ids) if unready: diff --git a/runbot_merge/models/project_freeze/views.xml b/runbot_merge/models/project_freeze/views.xml index 87b1baca..2e6ba031 100644 --- a/runbot_merge/models/project_freeze/views.xml +++ b/runbot_merge/models/project_freeze/views.xml @@ -27,7 +27,8 @@ - + diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index c0eae29d..c003d85d 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -531,6 +531,9 @@ class PullRequests(models.Model): url = fields.Char(compute='_compute_url') github_url = fields.Char(compute='_compute_url') + repo_name = fields.Char(related='repository.name') + message_title = fields.Char(compute='_compute_message_title') + @api.depends('repository.name', 'number') def _compute_url(self): base = werkzeug.urls.url_parse(self.env['ir.config_parameter'].sudo().get_param('web.base.url', 'http://localhost:8069')) @@ -540,15 +543,20 @@ class PullRequests(models.Model): pr.url = str(base.join(path)) pr.github_url = str(gh_base.join(path)) - @api.depends('repository.name', 'number') + @api.depends('message') + def _compute_message_title(self): + for pr in self: + pr.message_title = next(iter(pr.message.splitlines()), '') + + @api.depends('repository.name', 'number', 'message') def _compute_display_name(self): return super(PullRequests, self)._compute_display_name() def name_get(self): - return [ - (p.id, '%s#%d' % (p.repository.name, p.number)) - for p in self - ] + name_template = '%(repo_name)s#%(number)d' + if self.env.context.get('pr_include_title'): + name_template += ' (%(message_title)s)' + return [(p.id, name_template % p) for p in self] @api.model def name_search(self, name='', args=None, operator='ilike', limit=100): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 7b71d2f6..a51d99b7 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1112,10 +1112,10 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): # have 2 PRs required for the freeze with repo_a: - repo_a.make_commits('master', Commit('super important file', tree={'g': 'x'}), ref='heads/apr') + repo_a.make_commits(masters[0], Commit('super important file', tree={'g': 'x'}), ref='heads/apr') pr_required_a = repo_a.make_pr(target='master', head='apr') with repo_c: - repo_c.make_commits('master', Commit('update thing', tree={'f': '2'}), ref='heads/cpr') + repo_c.make_commits(masters[2], Commit('update thing', tree={'f': '2'}), ref='heads/cpr') pr_required_c = repo_c.make_pr(target='master', head='cpr') # have 3 release PRs, only the first one updates the tree (version file) @@ -1134,6 +1134,8 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): ) pr_rel_b = repo_b.make_pr(target='master', head='release-1.1') with repo_c: + repo_c.make_commits(masters[2], Commit("Some change", tree={'a': '1'}), ref='heads/whocares') + pr_other = repo_c.make_pr(target='master', head='whocares') repo_c.make_commits( masters[2], Commit('Release 1.1 (C)', tree=None), @@ -1147,7 +1149,6 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): repo_b.name: to_pr(env, pr_rel_b), repo_c.name: to_pr(env, pr_rel_c), } - # trigger the ~~tree~~ freeze wizard w = project.action_prepare_freeze() w2 = project.action_prepare_freeze() @@ -1164,9 +1165,14 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): # configure releases for r in w_id.release_pr_ids: r.pr_id = release_prs[r.repository_id.name].id + w_id.release_pr_ids[-1].pr_id = to_pr(env, pr_other).id r = w_id.action_freeze() assert r == w, "the freeze is not ready so the wizard should redirect to itself" - assert w_id.errors == "* 2 required PRs not ready." + owner = repo_c.owner + assert w_id.errors == f"""\ +* All release PRs must have the same label, found '{owner}:release-1.1, {owner}:whocares'. +* 2 required PRs not ready.""" + w_id.release_pr_ids[-1].pr_id = release_prs[repo_c.name].id with repo_a: pr_required_a.post_comment('hansen r+', config['role_reviewer']['token'])