[IMP] runbot_merge: small reorg of main models file

To prep for the addition of the freeze wizard:

* move projects out of `pull_requests.py`
* then realize half the methods there have no relation to projects and
  move them to more relevant places in `pull_requests.py`
* update corresponding crons (and tests using those crons) as the
  methods have changed model, and the cron definitions thus need to be
  updated
* split update to labels out of sending feedback comments while at it:
  labels are not used much during tests so their manipulation can be
  avoided; and labels are not as urgent as feedback so the crons can
  be quite a bit slower
* move the project view out of `mergebot.xml` as well

runbot_merge/__manifest__.py

@@ -8,6 +8,7 @@
 
         'data/merge_cron.xml',
         'views/res_partner.xml',
+        'views/runbot_merge_project.xml',
         'views/mergebot.xml',
         'views/templates.xml',
     ],

runbot_merge/data/merge_cron.xml

@@ -21,19 +21,29 @@
   </record>
   <record model="ir.cron" id="feedback_cron">
     <field name="name">Send feedback to PR</field>
-    <field name="model_id" ref="model_runbot_merge_project"/>
+    <field name="model_id" ref="model_runbot_merge_pull_requests_feedback"/>
     <field name="state">code</field>
-    <field name="code">model._send_feedback()</field>
+    <field name="code">model._send()</field>
     <field name="interval_number">1</field>
     <field name="interval_type">minutes</field>
     <field name="numbercall">-1</field>
     <field name="doall" eval="False"/>
   </record>
+  <record model="ir.cron" id="labels_cron">
+    <field name="name">Update labels on PR</field>
+    <field name="model_id" ref="model_runbot_merge_pull_requests_tagging"/>
+    <field name="state">code</field>
+    <field name="code">model._send()</field>
+    <field name="interval_number">10</field>
+    <field name="interval_type">minutes</field>
+    <field name="numbercall">-1</field>
+    <field name="doall" eval="False"/>
+  </record>
   <record model="ir.cron" id="fetch_prs_cron">
     <field name="name">Check for PRs to fetch</field>
-    <field name="model_id" ref="model_runbot_merge_project"/>
+    <field name="model_id" ref="model_runbot_merge_fetch_job"/>
     <field name="state">code</field>
-    <field name="code">model._check_fetch(True)</field>
+    <field name="code">model._check(True)</field>
     <field name="interval_number">1</field>
     <field name="interval_type">minutes</field>
     <field name="numbercall">-1</field>

runbot_merge/models/__init__.py

@@ -1,2 +1,3 @@
 from . import res_partner
+from . import project
 from . import pull_requests

runbot_merge/models/project.py

@@ -0,0 +1,83 @@
+import logging
+import re
+
+from odoo import models, fields
+
+_logger = logging.getLogger(__name__)
+class Project(models.Model):
+    _name = _description = 'runbot_merge.project'
+
+    name = fields.Char(required=True, index=True)
+    repo_ids = fields.One2many(
+        'runbot_merge.repository', 'project_id',
+        help="Repos included in that project, they'll be staged together. "\
+        "*Not* to be used for cross-repo dependencies (that is to be handled by the CI)"
+    )
+    branch_ids = fields.One2many(
+        'runbot_merge.branch', 'project_id',
+        context={'active_test': False},
+        help="Branches of all project's repos which are managed by the merge bot. Also "\
+        "target branches of PR this project handles."
+    )
+
+    ci_timeout = fields.Integer(
+        default=60, required=True,
+        help="Delay (in minutes) before a staging is considered timed out and failed"
+    )
+
+    github_token = fields.Char("Github Token", required=True)
+    github_prefix = fields.Char(
+        required=True,
+        default="hanson", # mergebot du bot du bot du~
+        help="Prefix (~bot name) used when sending commands from PR "
+             "comments e.g. [hanson retry] or [hanson r+ p=1]"
+    )
+
+    batch_limit = fields.Integer(
+        default=8, help="Maximum number of PRs staged together")
+
+    secret = fields.Char(
+        help="Webhook secret. If set, will be checked against the signature "
+             "of (valid) incoming webhook signatures, failing signatures "
+             "will lead to webhook rejection. Should only use ASCII."
+    )
+
+    def _check_stagings(self, commit=False):
+        for branch in self.search([]).mapped('branch_ids').filtered('active'):
+            staging = branch.active_staging_id
+            if not staging:
+                continue
+            try:
+                with self.env.cr.savepoint():
+                    staging.check_status()
+            except Exception:
+                _logger.exception("Failed to check staging for branch %r (staging %s)",
+                                  branch.name, staging)
+            else:
+                if commit:
+                    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()
+
+    def _find_commands(self, comment):
+        return re.findall(
+            '^\s*[@|#]?{}:? (.*)$'.format(self.github_prefix),
+            comment, re.MULTILINE | re.IGNORECASE)
+
+    def _has_branch(self, name):
+        self.env.cr.execute("""
+        SELECT 1 FROM runbot_merge_branch
+        WHERE project_id = %s AND name = %s
+        LIMIT 1
+        """, (self.id, name))
+        return bool(self.env.cr.rowcount)

runbot_merge/models/pull_requests.py

@@ -29,207 +29,7 @@ from .. import github, exceptions, controllers, utils
 WAIT_FOR_VISIBILITY = [10, 10, 10, 10]
 
 _logger = logging.getLogger(__name__)
-class Project(models.Model):
-    _name = _description = 'runbot_merge.project'
 
-    name = fields.Char(required=True, index=True)
-    repo_ids = fields.One2many(
-        'runbot_merge.repository', 'project_id',
-        help="Repos included in that project, they'll be staged together. "\
-        "*Not* to be used for cross-repo dependencies (that is to be handled by the CI)"
-    )
-    branch_ids = fields.One2many(
-        'runbot_merge.branch', 'project_id',
-        context={'active_test': False},
-        help="Branches of all project's repos which are managed by the merge bot. Also "\
-        "target branches of PR this project handles."
-    )
-
-    ci_timeout = fields.Integer(
-        default=60, required=True,
-        help="Delay (in minutes) before a staging is considered timed out and failed"
-    )
-
-    github_token = fields.Char("Github Token", required=True)
-    github_prefix = fields.Char(
-        required=True,
-        default="hanson", # mergebot du bot du bot du~
-        help="Prefix (~bot name) used when sending commands from PR "
-             "comments e.g. [hanson retry] or [hanson r+ p=1]"
-    )
-
-    batch_limit = fields.Integer(
-        default=8, help="Maximum number of PRs staged together")
-
-    secret = fields.Char(
-        help="Webhook secret. If set, will be checked against the signature "
-             "of (valid) incoming webhook signatures, failing signatures "
-             "will lead to webhook rejection. Should only use ASCII."
-    )
-
-    def _check_stagings(self, commit=False):
-        for branch in self.search([]).mapped('branch_ids').filtered('active'):
-            staging = branch.active_staging_id
-            if not staging:
-                continue
-            try:
-                with self.env.cr.savepoint():
-                    staging.check_status()
-            except Exception:
-                _logger.exception("Failed to check staging for branch %r (staging %s)",
-                                  branch.name, staging)
-            else:
-                if commit:
-                    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()
-
-    def _send_feedback(self):
-        Repos = self.env['runbot_merge.repository']
-        ghs = {}
-        # noinspection SqlResolve
-        self.env.cr.execute("""
-        SELECT
-            t.repository as repo_id,
-            t.pull_request as pr_number,
-            array_agg(t.id) as ids,
-            array_agg(t.tags_remove::json) as to_remove,
-            array_agg(t.tags_add::json) as to_add
-        FROM runbot_merge_pull_requests_tagging t
-        GROUP BY t.repository, t.pull_request
-        """)
-        to_remove = []
-        for repo_id, pr, ids, remove, add in self.env.cr.fetchall():
-            repo = Repos.browse(repo_id)
-
-            gh = ghs.get(repo)
-            if not gh:
-                gh = ghs[repo] = repo.github()
-
-            # fold all grouped PRs'
-            tags_remove, tags_add = set(), set()
-            for minus, plus in zip(remove, add):
-                tags_remove.update(minus)
-                # need to remove minuses from to_add in case we get e.g.
-                # -foo +bar; -bar +baz, if we don't remove the minus, we'll end
-                # up with -foo +bar +baz instead of -foo +baz
-                tags_add.difference_update(minus)
-                tags_add.update(plus)
-
-            try:
-                gh.change_tags(pr, tags_remove, tags_add)
-            except Exception:
-                _logger.exception(
-                    "Error while trying to change the tags of %s#%s from %s to %s",
-                    repo.name, pr, remove, add,
-                )
-            else:
-                to_remove.extend(ids)
-        self.env['runbot_merge.pull_requests.tagging'].browse(to_remove).unlink()
-
-        to_remove = []
-        for f in self.env['runbot_merge.pull_requests.feedback'].search([]):
-            repo = f.repository
-            gh = ghs.get((repo, f.token_field))
-            if not gh:
-                gh = ghs[(repo, f.token_field)] = repo.github(f.token_field)
-
-            try:
-                message = f.message
-                if f.close:
-                    gh.close(f.pull_request)
-                    try:
-                        data = json.loads(message or '')
-                    except json.JSONDecodeError:
-                        pass
-                    else:
-                        pr_to_notify = self.env['runbot_merge.pull_requests'].search([
-                            ('repository', '=', repo.id),
-                            ('number', '=', f.pull_request),
-                        ])
-                        if pr_to_notify:
-                            self._notify_pr_merged(gh, pr_to_notify, data)
-                            message = None
-                if message:
-                    gh.comment(f.pull_request, message)
-            except Exception:
-                _logger.exception(
-                    "Error while trying to %s %s#%s (%s)",
-                    'close' if f.close else 'send a comment to',
-                    repo.name, f.pull_request,
-                    utils.shorten(f.message, 200)
-                )
-            else:
-                to_remove.append(f.id)
-        self.env['runbot_merge.pull_requests.feedback'].browse(to_remove).unlink()
-
-    def _notify_pr_merged(self, gh, pr, payload):
-        deployment = gh('POST', 'deployments', json={
-            'ref': pr.head, 'environment': 'merge',
-            'description': "Merge %s into %s" % (pr, pr.target.name),
-            'task': 'merge',
-            'auto_merge': False,
-            'required_contexts': [],
-        }).json()
-        gh('POST', 'deployments/{}/statuses'.format(deployment['id']), json={
-            'state': 'success',
-            'target_url': 'https://github.com/{}/commit/{}'.format(
-                pr.repository.name,
-                payload['sha'],
-            ),
-            'description': "Merged %s in %s at %s" % (
-                pr, pr.target.name, payload['sha']
-            )
-        })
-
-    def is_timed_out(self, staging):
-        return fields.Datetime.from_string(staging.timeout_limit) < datetime.datetime.now()
-
-    def _check_fetch(self, commit=False):
-        """
-        :param bool commit: commit after each fetch has been executed
-        """
-        while True:
-            f = self.env['runbot_merge.fetch_job'].search([], limit=1)
-            if not f:
-                return
-
-            self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
-            try:
-                f.repository._load_pr(f.number)
-            except Exception:
-                self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
-                _logger.exception("Failed to load pr %s, skipping it", f.number)
-            finally:
-                self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch")
-
-            # commit after each fetched PR
-            f.active = False
-            if commit:
-                self.env.cr.commit()
-
-    def _find_commands(self, comment):
-        return re.findall(
-            '^\s*[@|#]?{}:? (.*)$'.format(self.github_prefix),
-            comment, re.MULTILINE | re.IGNORECASE)
-
-    def _has_branch(self, name):
-        self.env.cr.execute("""
-        SELECT 1 FROM runbot_merge_branch
-        WHERE project_id = %s AND name = %s
-        LIMIT 1
-        """, (self.id, name))
-        return bool(self.env.cr.rowcount)
 
 class StatusConfiguration(models.Model):
     _name = 'runbot_merge.repository.status'
@@ -1179,6 +979,25 @@ class PullRequests(models.Model):
             self.previous_failure = json.dumps(prev)
             self._notify_ci_failed(ci)
 
+    def _notify_merged(self, gh, payload):
+        deployment = gh('POST', 'deployments', json={
+            'ref': self.head, 'environment': 'merge',
+            'description': "Merge %s into %s" % (self.display_name, self.target.name),
+            'task': 'merge',
+            'auto_merge': False,
+            'required_contexts': [],
+        }).json()
+        gh('POST', 'deployments/{}/statuses'.format(deployment['id']), json={
+            'state': 'success',
+            'target_url': 'https://github.com/{}/commit/{}'.format(
+                self.repository.name,
+                payload['sha'],
+            ),
+            'description': "Merged %s in %s at %s" % (
+                self.display_name, self.target.name, payload['sha']
+            )
+        })
+
     def _statuses_equivalent(self, a, b):
         """ Check if two statuses are *equivalent* meaning the description field
         is ignored (check only state and target_url). This is because the
@@ -1613,6 +1432,49 @@ class Tagging(models.Model):
             values['tags_add'] = json.dumps(list(values['tags_add']))
         return super().create(values)
 
+    def _send(self):
+        # noinspection SqlResolve
+        self.env.cr.execute("""
+        SELECT
+            t.repository as repo_id,
+            t.pull_request as pr_number,
+            array_agg(t.id) as ids,
+            array_agg(t.tags_remove::json) as to_remove,
+            array_agg(t.tags_add::json) as to_add
+        FROM runbot_merge_pull_requests_tagging t
+        GROUP BY t.repository, t.pull_request
+        """)
+        Repos = self.env['runbot_merge.repository']
+        ghs = {}
+        to_remove = []
+        for repo_id, pr, ids, remove, add in self.env.cr.fetchall():
+            repo = Repos.browse(repo_id)
+
+            gh = ghs.get(repo)
+            if not gh:
+                gh = ghs[repo] = repo.github()
+
+            # fold all grouped PRs'
+            tags_remove, tags_add = set(), set()
+            for minus, plus in zip(remove, add):
+                tags_remove.update(minus)
+                # need to remove minuses from to_add in case we get e.g.
+                # -foo +bar; -bar +baz, if we don't remove the minus, we'll end
+                # up with -foo +bar +baz instead of -foo +baz
+                tags_add.difference_update(minus)
+                tags_add.update(plus)
+
+            try:
+                gh.change_tags(pr, tags_remove, tags_add)
+            except Exception:
+                _logger.exception(
+                    "Error while trying to change the tags of %s#%s from %s to %s",
+                    repo.name, pr, remove, add,
+                )
+            else:
+                to_remove.extend(ids)
+        self.browse(to_remove).unlink()
+
 class Feedback(models.Model):
     """ Queue of feedback comments to send to PR users
     """
@@ -1631,6 +1493,44 @@ class Feedback(models.Model):
         help="Token field (from repo's project) to use to post messages"
     )
 
+    def _send(self):
+        ghs = {}
+        to_remove = []
+        for f in self.search([]):
+            repo = f.repository
+            gh = ghs.get((repo, f.token_field))
+            if not gh:
+                gh = ghs[(repo, f.token_field)] = repo.github(f.token_field)
+
+            try:
+                message = f.message
+                if f.close:
+                    gh.close(f.pull_request)
+                    try:
+                        data = json.loads(message or '')
+                    except json.JSONDecodeError:
+                        pass
+                    else:
+                        pr_to_notify = self.env['runbot_merge.pull_requests'].search([
+                            ('repository', '=', repo.id),
+                            ('number', '=', f.pull_request),
+                        ])
+                        if pr_to_notify:
+                            pr_to_notify._notify_merged(gh, data)
+                            message = None
+                if message:
+                    gh.comment(f.pull_request, message)
+            except Exception:
+                _logger.exception(
+                    "Error while trying to %s %s#%s (%s)",
+                    'close' if f.close else 'send a comment to',
+                    repo.name, f.pull_request,
+                    utils.shorten(f.message, 200)
+                )
+            else:
+                to_remove.append(f.id)
+        self.browse(to_remove).unlink()
+
 class Commit(models.Model):
     """Represents a commit onto which statuses might be posted,
     independent of everything else as commits can be created by
@@ -1981,9 +1881,12 @@ class Stagings(models.Model):
             finally:
                 self.batch_ids.write({'active': False})
                 self.write({'active': False})
-        elif self.state == 'failure' or project.is_timed_out(self):
+        elif self.state == 'failure' or self.is_timed_out():
             self.try_splitting()
 
+    def is_timed_out(self):
+        return fields.Datetime.from_string(self.timeout_limit) < datetime.datetime.now()
+
     def _safety_dance(self, gh, staging_heads):
         """ Reverting updates doesn't work if the branches are protected
         (because a revert is basically a force push). So we can update
@@ -2154,6 +2057,28 @@ class FetchJob(models.Model):
     repository = fields.Many2one('runbot_merge.repository', required=True)
     number = fields.Integer(required=True)
 
+    def _check(self, commit=False):
+        """
+        :param bool commit: commit after each fetch has been executed
+        """
+        while True:
+            f = self.search([], limit=1)
+            if not f:
+                return
+
+            self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
+            try:
+                f.repository._load_pr(f.number)
+            except Exception:
+                self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
+                _logger.exception("Failed to load pr %s, skipping it", f.number)
+            finally:
+                self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch")
+
+            f.active = False
+            if commit:
+                self.env.cr.commit()
+
 # The commit (and PR) statuses was originally a map of ``{context:state}``
 # however it turns out to clarify error messages it'd be useful to have
 # a bit more information e.g. a link to the CI's build info on failure and

runbot_merge/tests/conftest.py

@@ -27,7 +27,7 @@ def default_crons():
         'runbot_merge.staging_cron',
         # env['runbot_merge.pull_requests']._check_linked_prs_statuses()
         'runbot_merge.check_linked_prs_status',
-        # env['runbot_merge.project']._send_feedback()
+        # env['runbot_merge.pull_requests.feedback']._send()
         'runbot_merge.feedback_cron',
     ]
 

runbot_merge/tests/test_basic.py

@@ -907,7 +907,7 @@ def test_rebase_failure(env, repo, users, config):
     with mock.patch.object(GH, 'set_ref', autospec=True, side_effect=wrapper):
         env['runbot_merge.project']._check_progress()
 
-    env['runbot_merge.project']._send_feedback()
+    env['runbot_merge.pull_requests.feedback']._send()
 
     assert pr_a.comments == [
         (users['reviewer'], 'hansen r+'),

runbot_merge/tests/test_by_branch.py

@@ -154,6 +154,7 @@ def test_pseudo_version_tag(env, project, make_repo, setreviewers, config):
     with repo:
         repo.post_status('staging.master', 'success', 'ci')
     env.run_crons() # should merge staging
+    env.run_crons('runbot_merge.labels_cron') # update labels
     assert pr_id.state == 'merged'
     assert pr.labels >= {'2.1'}
 
@@ -174,5 +175,6 @@ def test_pseudo_version_tag(env, project, make_repo, setreviewers, config):
     with repo:
         repo.post_status('staging.master', 'success', 'ci')
     env.run_crons() # should merge staging
+    env.run_crons('runbot_merge.labels_cron') # update labels
     assert pr_id.state == 'merged'
     assert pr.labels >= {'post-bonk'}

runbot_merge/views/mergebot.xml

@@ -1,50 +1,4 @@
 <odoo>
-    <record id="runbot_merge_form_project" model="ir.ui.view">
-        <field name="name">Project Form</field>
-        <field name="model">runbot_merge.project</field>
-        <field name="arch" type="xml">
-            <form>
-                <sheet>
-                    <div class="oe_title">
-                        <h1><field name="name" placeholder="Name"/></h1>
-                    </div>
-                    <group>
-                        <group>
-                            <field name="github_prefix" string="bot name"/>
-                        </group>
-                    </group>
-                    <group>
-                        <group>
-                            <field name="github_token"/>
-                            <field name="secret"/>
-                        </group>
-                        <group>
-                            <field name="ci_timeout"/>
-                            <field name="batch_limit"/>
-                        </group>
-                    </group>
-
-                    <separator string="Repositories"/>
-                    <field name="repo_ids">
-                        <tree>
-                            <field name="sequence" widget="handle"/>
-                            <field name="name"/>
-                            <field name="branch_filter"/>
-                            <field name="status_ids" widget="many2many_tags"/>
-                        </tree>
-                    </field>
-                    <separator string="Branches"/>
-                    <field name="branch_ids">
-                        <tree editable="bottom" decoration-muted="not active">
-                            <field name="sequence" widget="handle" />
-                            <field name="name"/>
-                            <field name="active"/>
-                        </tree>
-                    </field>
-                </sheet>
-            </form>
-        </field>
-    </record>
 
     <record id="form_repository" model="ir.ui.view">
         <field name="name">Repository form</field>

runbot_merge/views/runbot_merge_project.xml

@@ -0,0 +1,51 @@
+<odoo>
+    <record id="runbot_merge_form_project" model="ir.ui.view">
+        <field name="name">Project Form</field>
+        <field name="model">runbot_merge.project</field>
+        <field name="arch" type="xml">
+            <form>
+                <header>
+                    <button type="object" name="action_freeze" string="Freeze" class="oe_highlight"/>
+                </header>
+                <sheet>
+                    <div class="oe_title">
+                        <h1><field name="name" placeholder="Name"/></h1>
+                    </div>
+                    <group>
+                        <group>
+                            <field name="github_prefix" string="bot name"/>
+                        </group>
+                    </group>
+                    <group>
+                        <group>
+                            <field name="github_token"/>
+                            <field name="secret"/>
+                        </group>
+                        <group>
+                            <field name="ci_timeout"/>
+                            <field name="batch_limit"/>
+                        </group>
+                    </group>
+
+                    <separator string="Repositories"/>
+                    <field name="repo_ids">
+                        <tree>
+                            <field name="sequence" widget="handle"/>
+                            <field name="name"/>
+                            <field name="branch_filter"/>
+                            <field name="status_ids" widget="many2many_tags"/>
+                        </tree>
+                    </field>
+                    <separator string="Branches"/>
+                    <field name="branch_ids">
+                        <tree editable="bottom" decoration-muted="not active">
+                            <field name="sequence" widget="handle" />
+                            <field name="name"/>
+                            <field name="active"/>
+                        </tree>
+                    </field>
+                </sheet>
+            </form>
+        </field>
+    </record>
+</odoo>