From 9c51f87aed36fc1f310e3854124d8767f65d871f Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 3 Jun 2024 12:59:24 +0200 Subject: [PATCH] [ADD] runbot_merge: support for non-webhook staging validation Add support for the ability to validate *stagings* over RPC rather than via webhook. This may later be expanded to PRs as well. The core motivation for this is to avoid bouncing through github which sometimes drops the ball on statuses, and it's frustrating to have a staging time out because GH fucked up. Implemented via RPC, requiring both the staging itself (by id) and the head commit being affected, as that is necessary to know what CIs are required for that head and correctly report cross branch on the various PRs. Fix #881 (kinda) --- runbot_merge/models/project.py | 1 + runbot_merge/models/pull_requests.py | 35 +++++-- runbot_merge/security/security.xml | 3 + runbot_merge/tests/test_basic.py | 109 ++++++++++++-------- runbot_merge/views/runbot_merge_project.xml | 10 ++ 5 files changed, 107 insertions(+), 51 deletions(-) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index dee87735..db41758b 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -33,6 +33,7 @@ class Project(models.Model): ('ready', "Ready PRs over split"), ], default="default", required=True) staging_statuses = fields.Boolean(default=True) + staging_rpc = fields.Boolean(default=False) ci_timeout = fields.Integer( default=60, required=True, group_operator=None, diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index a8716ba7..2055a2cf 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -18,6 +18,7 @@ import sentry_sdk import werkzeug from odoo import api, fields, models, tools, Command +from odoo.exceptions import AccessError from odoo.osv import expression from odoo.tools import html_escape, Reverse from . import commands @@ -1813,14 +1814,6 @@ class Stagings(models.Model): statuses = fields.Binary(compute='_compute_statuses') statuses_cache = fields.Text(default='{}', required=True) - def write(self, vals): - # don't allow updating the statuses_cache - vals.pop('statuses_cache', None) - - super().write(vals) - - return True - @api.depends('staged_at', 'staging_end') def _compute_duration(self): for s in self: @@ -1896,6 +1889,27 @@ class Stagings(models.Model): """, {'sha': c.sha, 'statuses': c.statuses, 'ids': self.ids}) self.modified(['statuses_cache']) + def post_status(self, sha, context, status, *, target_url=None, description=None): + if not self.env.user.has_group('runbot_merge.status'): + raise AccessError("You are not allowed to post a status.") + + for s in self: + if not s.target.project_id.staging_rpc: + continue + + if not any(c.commit_id.sha == sha for c in s.commits): + raise ValueError(f"Staging {s.id} does not have the commit {sha}") + + st = json.loads(s.statuses_cache) + st.setdefault(sha, {})[context] = { + 'state': status, + 'target_url': target_url, + 'description': description, + } + s.statuses_cache = json.dumps(st) + + return True + @api.depends( "statuses_cache", "target", @@ -2006,17 +2020,18 @@ class Stagings(models.Model): }) return True - # single batch => the staging is an unredeemable failure + # single batch => the staging is an irredeemable failure if self.state != 'failure': # timed out, just mark all PRs (wheee) self.fail('timed out (>{} minutes)'.format(self.target.project_id.ci_timeout)) return False + staging_statuses = json.loads(self.statuses_cache) # try inferring which PR failed and only mark that one for head in self.heads: required_statuses = set(head.repository_id.status_ids._for_staging(self).mapped('context')) - statuses = json.loads(head.commit_id.statuses or '{}') + statuses = staging_statuses.get(head.commit_id.sha, {}) reason = next(( ctx for ctx, result in statuses.items() if ctx in required_statuses diff --git a/runbot_merge/security/security.xml b/runbot_merge/security/security.xml index 62e1f323..d1ea1c79 100644 --- a/runbot_merge/security/security.xml +++ b/runbot_merge/security/security.xml @@ -5,4 +5,7 @@ + + Mergebot Status Sender + diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index afa7a3a9..9299738a 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -24,6 +24,41 @@ def repo(env, project, make_repo, users, setreviewers): setreviewers(*project.repo_ids) return r +@pytest.fixture(autouse=True, params=["statuses", "rpc"]) +def stagings(request, env, project, repo): + """Hook in support for validating stagings via RPC calls instead of CI + webhooks. Transparent for the tests as long as they send statuses to + symbolic refs (branch names) rather than commits, although commits *would* + probably be doable (look up the head for the commit, then what staging it's + part of) + """ + if request.param == "statuses": + yield + else: + env['res.users'].browse([env._uid]).write({ + "groups_id": [(4, env.ref("runbot_merge.status").id, {})] + }) + project.write({ + "staging_rpc": True, + "staging_statuses": False, + }) + RepoType = type(repo) + # apparently side_effect + wraps on unbound method don't work correctly, + # the wrapped method does get called when returning DEFAULT but *the + # instance (subject) is not sent along for the ride* so the call fails. + post_status = RepoType.post_status + def _post_status(repo, ref, status, context='default', **kw): + if not ref.startswith(('staging.', 'heads/staging.')): + return post_status(repo, ref, status, context, **kw) + + c = repo.commit(ref) + branchname = ref.removeprefix('staging.').removeprefix('heads/staging.') + env['runbot_merge.stagings'].search([('target.name', '=', branchname)])\ + .post_status(c.id, context, status, **kw) + + with mock.patch.object(RepoType, "post_status", _post_status): + yield + def test_trivial_flow(env, repo, page, users, config): # create base branch with repo: @@ -81,12 +116,10 @@ def test_trivial_flow(env, repo, page, users, config): assert pr_page(page, pr).cssselect('.alert-primary') with repo: - # get head of staging branch - staging_head = repo.commit('heads/staging.master') - repo.post_status(staging_head.id, 'success', 'ci/runbot', target_url='http://foo.com/pog') - repo.post_status(staging_head.id, 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot', target_url='http://foo.com/pog') + repo.post_status('staging.master', 'success', 'legal/cla') # the should not block the merge because it's not part of the requirements - repo.post_status(staging_head.id, 'failure', 'ci/lint', target_url='http://ignored.com/whocares') + repo.post_status('staging.master', 'failure', 'ci/lint', target_url='http://ignored.com/whocares') # need to store this because after the crons have run the staging will # have succeeded and been disabled st = pr_id.staging_id @@ -404,18 +437,16 @@ def test_staging_ongoing(env, repo, config): ]) assert p_2.state == 'ready', "PR2 should not have been staged since there is a pending staging for master" - staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status(staging_head.id, 'success', 'ci/runbot') - repo.post_status(staging_head.id, 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') env.run_crons() assert pr1.state == 'merged' assert p_2.staging_id - staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status(staging_head.id, 'success', 'ci/runbot') - repo.post_status(staging_head.id, 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') env.run_crons() assert p_2.state == 'merged' @@ -570,7 +601,7 @@ def test_timeout_bump_on_pending(env, repo, config): old_timeout = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(days=15)) st.timeout_limit = old_timeout with repo: - repo.post_status(repo.commit('heads/staging.master').id, 'pending', 'ci/runbot') + repo.post_status('staging.master', 'pending', 'ci/runbot') env.run_crons('runbot_merge.process_updated_commits') assert st.timeout_limit > old_timeout @@ -591,11 +622,10 @@ def test_staging_ci_failure_single(env, repo, users, config, page): pr_id = to_pr(env, pr) assert pr_id.staging_id - staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status(staging_head.id, 'failure', 'a/b') - repo.post_status(staging_head.id, 'success', 'legal/cla') - repo.post_status(staging_head.id, 'failure', 'ci/runbot') # stable genius + repo.post_status('staging.master', 'failure', 'a/b') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') # stable genius env.run_crons() assert pr_id.state == 'error' @@ -636,8 +666,8 @@ def test_ff_failure(env, repo, config, page): # report staging success & run cron to merge staging = repo.commit('heads/staging.master') with repo: - repo.post_status(staging.id, 'success', 'legal/cla') - repo.post_status(staging.id, 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') env.run_crons() assert st.reason == 'update is not a fast forward' @@ -933,8 +963,8 @@ def test_forward_port(env, repo, config): st = repo.commit('staging.master') with repo: - repo.post_status(st.id, 'success', 'legal/cla') - repo.post_status(st.id, 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') env.run_crons() h = repo.commit('master') @@ -1158,8 +1188,8 @@ class TestRetry: ]).staging_id staging_head = repo.commit('heads/staging.master') - repo.post_status(staging_head.id, 'success', 'legal/cla') - repo.post_status(staging_head.id, 'failure', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') env.run_crons() pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1178,8 +1208,8 @@ class TestRetry: staging_head2 = repo.commit('heads/staging.master') assert staging_head2 != staging_head - repo.post_status(staging_head2.id, 'success', 'legal/cla') - repo.post_status(staging_head2.id, 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') env.run_crons() assert pr.state == 'merged' @@ -1202,8 +1232,8 @@ class TestRetry: staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status(staging_head.id, 'success', 'legal/cla') - repo.post_status(staging_head.id, 'failure', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') env.run_crons() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1221,8 +1251,8 @@ class TestRetry: staging_head2 = repo.commit('heads/staging.master') assert staging_head2 != staging_head with repo: - repo.post_status(staging_head2.id, 'success', 'legal/cla') - repo.post_status(staging_head2.id, 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') env.run_crons() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1297,8 +1327,8 @@ class TestRetry: staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status(staging_head.id, 'success', 'legal/cla') - repo.post_status(staging_head.id, 'failure', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') env.run_crons() pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1357,8 +1387,8 @@ class TestMergeMethod: "dummy commit aside, the previous master's tip should be the sole parent of the staging commit" with repo: - repo.post_status(staging.id, 'success', 'legal/cla') - repo.post_status(staging.id, 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') env.run_crons() pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -2367,10 +2397,9 @@ class TestPRUpdate(object): assert pr.state == 'ready' assert pr.staging_id - h = repo.commit('heads/staging.master').id with repo: - repo.post_status(h, 'success', 'legal/cla') - repo.post_status(h, 'failure', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') env.run_crons() assert not pr.staging_id assert pr.state == 'error' @@ -3052,19 +3081,17 @@ class TestBatching(object): assert len(sp) == 1 # This is the failing PR! - h = repo.commit('heads/staging.master').id with repo: - repo.post_status(h, 'failure', 'ci/runbot') - repo.post_status(h, 'success', 'legal/cla') + repo.post_status('staging.master', 'failure', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') env.run_crons() assert pr1.state == 'error' assert pr2.staging_id - h = repo.commit('heads/staging.master').id with repo: - repo.post_status(h, 'success', 'ci/runbot') - repo.post_status(h, 'success', 'legal/cla') + repo.post_status('staging.master', 'success', 'ci/runbot') + repo.post_status('staging.master', 'success', 'legal/cla') env.run_crons('runbot_merge.process_updated_commits', 'runbot_merge.merge_cron', 'runbot_merge.staging_cron') assert pr2.state == 'merged' diff --git a/runbot_merge/views/runbot_merge_project.xml b/runbot_merge/views/runbot_merge_project.xml index d7b9324f..68eb28cc 100644 --- a/runbot_merge/views/runbot_merge_project.xml +++ b/runbot_merge/views/runbot_merge_project.xml @@ -30,6 +30,16 @@ + + Avoid overlaps between GH and RPC as the older + GH statuses may overwrite more recent RPC statuses. + + +