From 83511f45e2666ce59781b7f84148dc32b7c65c7a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Feb 2024 15:05:33 +0100 Subject: [PATCH] [CHG] runbot_merge: move priority field from PR to batch Simplifies the `ready_prs` query a bit and allows it to be converted to an ORM search, by moving the priority check outside. This also allows the caller to not need to post-process the records list anywhere near the previous state of affairs. `ready_prs` now returns *either* the "alone" batches, or the non-alone batches, rather than mixing both into a single sequence. This requires correctly applying the search filters to not retrieve priority of batches in error or targeting other branches. --- runbot_merge/models/batch.py | 31 +++++++++++++++++++++ runbot_merge/models/pull_requests.py | 18 ++++-------- runbot_merge/models/stagings_create.py | 38 +++++++++++++------------- runbot_merge/models/utils.py | 6 ++++ runbot_merge/tests/test_basic.py | 6 ++++ 5 files changed, 67 insertions(+), 32 deletions(-) create mode 100644 runbot_merge/models/utils.py diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index bb49c857..d3d2aacd 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -1,6 +1,8 @@ from __future__ import annotations from odoo import models, fields, api +from odoo.tools import create_index +from .utils import enum class Batch(models.Model): @@ -36,9 +38,38 @@ class Batch(models.Model): default=False, tracking=True, help="Cancels current staging on target branch when becoming ready" ) + priority = fields.Selection([ + ('default', "Default"), + ('priority', "Priority"), + ('alone', "Alone"), + ], default='default', index=True, group_operator=None, required=True, + column_type=enum(_name, 'priority'), + ) blocked = fields.Char(store=True, compute="_compute_stageable") + def _auto_init(self): + for field in self._fields.values(): + if not isinstance(field, fields.Selection) or field.column_type[0] == 'varchar': + continue + + t = field.column_type[1] + self.env.cr.execute("SELECT FROM pg_type WHERE typname = %s", [t]) + if not self.env.cr.rowcount: + self.env.cr.execute( + f"CREATE TYPE {t} AS ENUM %s", + [tuple(s for s, _ in field.selection)] + ) + + super()._auto_init() + + create_index( + self.env.cr, + "runbot_merge_batch_unblocked_idx", + "runbot_merge_batch", + ["(blocked is null), priority"], + ) + @api.depends( "merge_date", "prs.error", "prs.draft", "prs.squash", "prs.merge_method", diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 931ade1c..fb34361a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -10,7 +10,7 @@ import logging import re import time from functools import reduce -from typing import Optional, Union, List, Iterator, Tuple +from typing import Optional, Union, List, Iterator import sentry_sdk import werkzeug @@ -19,6 +19,7 @@ from odoo import api, fields, models, tools, Command from odoo.osv import expression from odoo.tools import html_escape from . import commands +from .utils import enum from .. import github, exceptions, controllers, utils @@ -307,9 +308,6 @@ class Branch(models.Model): ACL = collections.namedtuple('ACL', 'is_admin is_reviewer is_author') -def enum(model: str, field: str) -> Tuple[str, str]: - n = f'{model.replace(".", "_")}_{field}_type' - return n, n class PullRequests(models.Model): _name = 'runbot_merge.pull_requests' _description = "Pull Request" @@ -374,13 +372,7 @@ class PullRequests(models.Model): reviewed_by = fields.Many2one('res.partner', index=True, tracking=True) delegates = fields.Many2many('res.partner', help="Delegate reviewers, not intrinsically reviewers but can review this PR") - priority = fields.Selection([ - ('default', "Default"), - ('priority', "Priority"), - ('alone', "Alone"), - ], default='default', index=True, group_operator=None, required=True, - column_type=enum(_name, 'priority'), - ) + priority = fields.Selection(related="batch_id.priority", inverse=lambda _: 1 / 0) overrides = fields.Char(required=True, default='{}', tracking=True) statuses = fields.Text(help="Copy of the statuses from the HEAD commit, as a Python literal", default="{}") @@ -755,7 +747,7 @@ class PullRequests(models.Model): }) delegates.write({'delegate_reviewer': [(4, self.id, 0)]}) case commands.Priority() if is_admin: - self.priority = str(command) + self.batch_id.priority = str(command) case commands.SkipChecks() if is_admin: self.batch_id.skipchecks = True self.reviewed_by = author @@ -1014,7 +1006,7 @@ class PullRequests(models.Model): [tuple(s for s, _ in field.selection)] ) - super(PullRequests, self)._auto_init() + super()._auto_init() # incorrect index: unique(number, target, repository). tools.drop_index(self._cr, 'runbot_merge_unique_pr_per_target', self._table) # correct index: diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index fc893937..231ee969 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -61,11 +61,9 @@ def try_staging(branch: Branch) -> Optional[Stagings]: def log(label: str, batches: Batch) -> None: _logger.info(label, ', '.join(batches.mapped('prs.display_name'))) - rows = ready_prs(for_branch=branch) - priority = rows[0][0] if rows else None - concat = branch.env['runbot_merge.batch'].concat - if priority == 'alone': - batches: Batch = concat(*(batch for _, batch in takewhile(lambda r: r[0] == priority, rows))) + alone, batches = ready_prs(for_branch=branch) + print('alone', alone, 'ready', batches) + if alone: log("staging high-priority PRs %s", batches) elif branch.project_id.staging_priority == 'default': if split := branch.split_ids[:1]: @@ -75,7 +73,6 @@ def try_staging(branch: Branch) -> Optional[Stagings]: else: # priority, normal; priority = sorted ahead of normal, so always picked # first as long as there's room - batches = concat(*(batch for _, batch in rows)) log("staging ready PRs %s (prioritising splits)", batches) elif branch.project_id.staging_priority == 'ready': # splits are ready by definition, we need to exclude them from the @@ -83,7 +80,7 @@ def try_staging(branch: Branch) -> Optional[Stagings]: # we cycle forever # FIXME: once the batches are less shit, store this durably on the # batches and filter out when fetching readies (?) - batches = concat(*(batch for _, batch in rows)) - branch.split_ids.batch_ids + batches -= branch.split_ids.batch_ids if batches: log("staging ready PRs %s (prioritising ready)", batches) @@ -97,7 +94,7 @@ def try_staging(branch: Branch) -> Optional[Stagings]: # splits are ready by definition, we need to exclude them from the # ready rows otherwise ready always wins but we re-stage the splits, so # if an error is legit we'll cycle forever - batches = concat(*(batch for _, batch in rows)) - branch.split_ids.batch_ids + batches -= branch.split_ids.batch_ids maxsplit = max(branch.split_ids, key=lambda s: len(s.batch_ids), default=branch.env['runbot_merge.split']) _logger.info("largest split = %d, ready = %d", len(maxsplit.batch_ids), len(batches)) @@ -204,20 +201,23 @@ For-Commit-Id: {it.head} return st -def ready_prs(for_branch: Branch) -> List[Tuple[int, Batch]]: +def ready_prs(for_branch: Branch) -> Tuple[bool, Batch]: env = for_branch.env env.cr.execute(""" - SELECT - max(pr.priority) as priority, - b.id as batch - FROM runbot_merge_batch b - JOIN runbot_merge_pull_requests pr ON (b.id = pr.batch_id) - WHERE b.target = %s AND b.blocked IS NULL - GROUP BY b.id - ORDER BY max(pr.priority) DESC, min(b.id) + SELECT max(priority) + FROM runbot_merge_batch + WHERE blocked IS NULL AND target = %s """, [for_branch.id]) - browse = env['runbot_merge.batch'].browse - return [(p, browse(id)) for p, id in env.cr.fetchall()] + alone = env.cr.fetchone()[0] == 'alone' + + return ( + alone, + env['runbot_merge.batch'].search([ + ('target', '=', for_branch.id), + ('blocked', '=', False), + ('priority', '=', 'alone') if alone else (1, '=', 1), + ], order="priority DESC, id ASC"), + ) def staging_setup( diff --git a/runbot_merge/models/utils.py b/runbot_merge/models/utils.py new file mode 100644 index 00000000..6aeeb9b8 --- /dev/null +++ b/runbot_merge/models/utils.py @@ -0,0 +1,6 @@ +from typing import Tuple + + +def enum(model: str, field: str) -> Tuple[str, str]: + n = f'{model.replace(".", "_")}_{field}_type' + return n, n diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 4ca95c60..273c0aae 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2820,11 +2820,17 @@ class TestBatching(object): assert not staging_1.active assert not p_11.staging_id and not p_12.staging_id assert p_01.staging_id + assert p_11.state == 'ready' + assert p_12.state == 'ready' # make the staging fail with repo: repo.post_status('staging.master', 'failure', 'ci/runbot') env.run_crons() + assert p_01.error + assert p_01.batch_id.blocked + assert p_01.blocked + assert p_01.state == 'error' assert not p_01.staging_id.active staging_2 = ensure_one(sm_all.staging_id)