[MRG] runbot_merge, forwardport: multiple fixes to freeze wizard

Multiple fixes to various issues of the freeze wizard.

Some of the less fix-oriented sub-tasks were moved to #586 for discussion instead.

Closes #559, #587

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'
 

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')

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

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"

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',

runbot_merge/models/project.py

@@ -114,6 +114,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_freeze()
+        return w.action_open()

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:
@@ -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"
@@ -94,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
@@ -114,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)
@@ -163,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

runbot_merge/models/project_freeze/views.xml

@@ -25,9 +25,10 @@
                     <group>
                         <group colspan="2">
                             <field name="release_pr_ids" nolabel="1">
-                                <tree editable="bottom" create="false" unlink="false">
+                                <tree editable="bottom" create="false">
                                     <field name="repository_id" readonly="1"/>
-                                    <field name="pr_id" options="{'no_create': True}"/>
+                                    <field name="pr_id" options="{'no_create': True}"
+                                           context="{'pr_include_title': 1}"/>
                                 </tree>
                             </field>
                         </group>
@@ -49,4 +50,15 @@
             </form>
         </field>
     </record>
+
+    <record id="runbot_merge_repository_freeze" model="ir.ui.view">
+        <field name="name">Add freeze field to repo form</field>
+        <field name="model">runbot_merge.repository</field>
+        <field name="inherit_id" ref="form_repository"/>
+        <field name="arch" type="xml">
+            <field name="branch_filter" position="after">
+                <field name="freeze"/>
+            </field>
+        </field>
+    </record>
 </odoo>

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):

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

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,11 +1149,11 @@ 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()
     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"
@@ -1163,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'])
@@ -1188,6 +1195,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()
@@ -1225,3 +1240,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