From 0e0348e4df9e8aae73170db03e5e1b407e7dca9f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 17 May 2024 10:51:16 +0200 Subject: [PATCH] [IMP] runbot_merge: preserve batch ordering in stagings Batch ordering in stagings is important in order to correctly reconstitute the full project history. In the old mergebot, since batches are created on the fly during staging this information is reified by the batch ids. But since batch ids are now persistent and there is no relationship between the creation of a batch and its merging (especially not relative to other batches) it's an issue as reconstituting sub-staging git history would be impossible. Which is not the worst, but is not great. The solution is to reify the join table between stagings and batches in order for *that* to keep history (simply via the sequential PK), and in converting to the new system carefully generate the new links in an order matching the old batch ids. --- runbot_merge/models/batch.py | 39 ++++++++++++++++++++++- runbot_merge/models/pull_requests.py | 8 ++++- runbot_merge/models/stagings_create.py | 5 ++- runbot_merge/security/ir.model.access.csv | 1 + runbot_merge/tests/test_basic.py | 9 ++++++ 5 files changed, 57 insertions(+), 5 deletions(-) diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index 0a7822d4..e48f0541 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -9,6 +9,7 @@ from collections import defaultdict from collections.abc import Iterator import requests +from psycopg2 import sql from odoo import models, fields, api from .utils import enum @@ -18,6 +19,28 @@ _logger = logging.getLogger(__name__) FOOTER = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n' +class StagingBatch(models.Model): + _name = 'runbot_merge.staging.batch' + _description = "link between batches and staging in order to maintain an " \ + "ordering relationship between the batches of a staging" + _log_access = False + _order = 'id' + + runbot_merge_batch_id = fields.Many2one('runbot_merge.batch', required=True) + runbot_merge_stagings_id = fields.Many2one('runbot_merge.stagings', required=True) + + def init(self): + super().init() + + self.env.cr.execute(sql.SQL(""" + CREATE UNIQUE INDEX IF NOT EXISTS runbot_merge_staging_batch_idx + ON {table} (runbot_merge_stagings_id, runbot_merge_batch_id); + + CREATE INDEX IF NOT EXISTS runbot_merge_staging_batch_rev + ON {table} (runbot_merge_batch_id) INCLUDE (runbot_merge_stagings_id); + """).format(table=sql.Identifier(self._table))) + + class Batch(models.Model): """ A batch is a "horizontal" grouping of *codependent* PRs: PRs with the same label & target but for different repositories. These are @@ -31,7 +54,12 @@ class Batch(models.Model): _parent_store = True target = fields.Many2one('runbot_merge.branch', store=True, compute='_compute_target') - staging_ids = fields.Many2many('runbot_merge.stagings') + batch_staging_ids = fields.One2many('runbot_merge.staging.batch', 'runbot_merge_batch_id') + staging_ids = fields.Many2many( + 'runbot_merge.stagings', + compute="_compute_staging_ids", + context={'active_test': False}, + ) split_id = fields.Many2one('runbot_merge.split', index=True) prs = fields.One2many('runbot_merge.pull_requests', 'batch_id') @@ -74,6 +102,11 @@ class Batch(models.Model): context={"active_test": False}, ) + @api.depends('batch_staging_ids.runbot_merge_stagings_id') + def _compute_staging_ids(self): + for batch in self: + batch.staging_ids = batch.batch_staging_ids.runbot_merge_stagings_id + @property def source(self): return self.browse(map(int, self.parent_path.split('/', 1)[:1])) @@ -452,6 +485,10 @@ class Batch(models.Model): return True + @api.ondelete(at_uninstall=True) + def _on_delete_clear_stagings(self): + self.batch_staging_ids.unlink() + def unlink(self): """ batches can be unlinked if they: diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index edbecd18..b07b031c 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1763,7 +1763,8 @@ class Stagings(models.Model): target = fields.Many2one('runbot_merge.branch', required=True, index=True) - batch_ids = fields.Many2many('runbot_merge.batch', context={'active_test': False}) + staging_batch_ids = fields.One2many('runbot_merge.staging.batch', 'runbot_merge_stagings_id') + batch_ids = fields.Many2many('runbot_merge.batch', context={'active_test': False}, compute="_compute_batch_ids") pr_ids = fields.One2many('runbot_merge.pull_requests', compute='_compute_prs') state = fields.Selection([ ('success', 'Success'), @@ -1814,6 +1815,11 @@ class Stagings(models.Model): for staging in self ] + @api.depends('staging_batch_ids.runbot_merge_batch_id') + def _compute_batch_ids(self): + for staging in self: + staging.batch_ids = staging.staging_batch_ids.runbot_merge_batch_id + @api.depends('heads') def _compute_statuses(self): """ Fetches statuses associated with the various heads, returned as diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 017b63a9..1f620154 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -8,13 +8,12 @@ import os import re from collections.abc import Mapping from difflib import Differ -from itertools import takewhile from operator import itemgetter from typing import Dict, Union, Optional, Literal, Callable, Iterator, Tuple, List, TypeAlias from werkzeug.datastructures import Headers -from odoo import api, models, fields +from odoo import api, models, fields, Command from odoo.tools import OrderedSet, groupby from .pull_requests import Branch, Stagings, PullRequests, Repository from .batch import Batch @@ -166,7 +165,7 @@ For-Commit-Id: {it.head} # create actual staging object st: Stagings = env['runbot_merge.stagings'].create({ 'target': branch.id, - 'batch_ids': [(4, batch.id, 0) for batch in staged], + 'staging_batch_ids': [Command.create({'runbot_merge_batch_id': batch.id}) for batch in staged], 'heads': heads, 'commits': commits, }) diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 5b2e102b..28f2bd7e 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -16,6 +16,7 @@ access_runbot_merge_stagings_commits_admin,Admin access to staging commits,model access_runbot_merge_stagings_cancel_admin,Admin access to cancelling stagings,model_runbot_merge_stagings_cancel,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_split_admin,Admin access to splits,model_runbot_merge_split,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_batch_admin,Admin access to batches,model_runbot_merge_batch,runbot_merge.group_admin,1,1,1,1 +access_runbot_merge_staging_batch_admin,Admin access to batch/staging link,model_runbot_merge_staging_batch,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_fetch_job_admin,Admin access to fetch jobs,model_runbot_merge_fetch_job,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_pull_requests_feedback_admin,Admin access to feedback,model_runbot_merge_pull_requests_feedback,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_review_rights,Admin access to review permissions,model_res_partner_review,runbot_merge.group_admin,1,1,1,1 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 77fb3859..e47ff180 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2784,6 +2784,11 @@ class TestBatching(object): staging = ensure_one(env['runbot_merge.stagings'].search([])) assert staging.pr_ids == pr11 | pr12 | pr21 + assert list(staging.batch_ids) == [ + pr11.batch_id, + pr12.batch_id, + pr21.batch_id, + ] assert not pr22.staging_id def test_batching_urgent(self, env, repo, config): @@ -2804,6 +2809,10 @@ class TestBatching(object): staging_1 = sm_all.staging_id assert staging_1 assert len(staging_1) == 1 + assert list(staging_1.batch_ids) == [ + p_11.batch_id, + p_12.batch_id, + ] # no statuses run on PR0s with repo: