From fec3d39d1965d32403f9072242e1bb163e6ec733 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 6 Jun 2024 11:07:57 +0200 Subject: [PATCH] [ADD] *: per-repository webhook secret Currently webhook secrets are configured per *project* which is an issue both because different repositories may have different administrators and thus creates safety concerns, and because multiple repositories can feed into different projects (e.g. on mergebot, odoo-dev/odoo is both an ancillary repository to the main RD project, and the main repository to the minor / legacy master-wowl project). This means it can be necessary to have multiple projects share the same secret as well, this then mandates the secret for more repositories per (1). This is a pain in the ass, so just detach secrets from projects and link them *only* to repositories, it's cleaner and easier to manage and set up progressively. This requires a lot of changes to the tests, as they all need to correctly configure the signaling. For `runbot_merge` there was *some* setup sharing already via the module-level `repo` fixtures`, those were merged into a conftest-level fixture which could handle the signaling setup. A few tests which unnecessarily set up repositories ad-hoc were also moved to the fixture. But for most of the ad-hoc setup in `runbot_merge`, as well as `forwardport` where it's all ad-hoc, events sources setup was just appended as is. This should probably be cleaned up at one point, with the various requirements collected and organised into a small set of fixtures doing the job more uniformly. Fixes #887 --- conftest.py | 4 ++ forwardport/tests/test_limit.py | 1 + forwardport/tests/test_simple.py | 1 - forwardport/tests/test_updates.py | 1 + forwardport/tests/test_weird.py | 3 ++ mergebot_test_utils/utils.py | 1 + runbot_merge/__manifest__.py | 2 +- runbot_merge/controllers/__init__.py | 25 ++++++---- .../migrations/15.0.1.14/pre-migration.py | 12 +++++ runbot_merge/models/__init__.py | 1 + runbot_merge/models/events_sources.py | 12 +++++ runbot_merge/models/project.py | 6 --- runbot_merge/security/ir.model.access.csv | 2 + runbot_merge/tests/conftest.py | 27 ++++++++++ runbot_merge/tests/test_basic.py | 26 ++++------ runbot_merge/tests/test_batch_consistency.py | 29 ++--------- runbot_merge/tests/test_by_branch.py | 35 ++++++------- runbot_merge/tests/test_disabled_branch.py | 3 ++ runbot_merge/tests/test_multirepo.py | 21 +++++--- runbot_merge/tests/test_oddities.py | 50 ++++--------------- runbot_merge/tests/test_project_toggles.py | 11 ---- runbot_merge/tests/test_staging.py | 13 ----- runbot_merge/tests/test_status_overrides.py | 3 ++ runbot_merge/views/configuration.xml | 17 +++++++ runbot_merge/views/runbot_merge_project.xml | 1 - 25 files changed, 154 insertions(+), 153 deletions(-) create mode 100644 runbot_merge/migrations/15.0.1.14/pre-migration.py create mode 100644 runbot_merge/models/events_sources.py diff --git a/conftest.py b/conftest.py index db3822f7..946c9466 100644 --- a/conftest.py +++ b/conftest.py @@ -452,6 +452,10 @@ def check(response): # to) break the existing local tests @pytest.fixture def make_repo(capsys, request, config, tunnel, users): + """Fixtures which creates a repository on the github side, plugs webhooks + in, and registers the repository for deletion on cleanup (unless + ``--no-delete`` is set) + """ owner = config['github']['owner'] github = requests.Session() github.headers['Authorization'] = 'token %s' % config['github']['token'] diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 071d2a54..673a2bcf 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -421,6 +421,7 @@ def post_merge(env, config, users, make_repo, branches): }) ] }) + env['runbot_merge.events_sources'].create({'repository': prod.name}) env['res.partner'].search([ ('github_login', '=', config['role_reviewer']['user']) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 901b93a7..eae53ce4 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -35,7 +35,6 @@ def test_straightforward_flow(env, config, make_repo, users): other_user = config['role_other'] other_user_repo = prod.fork(token=other_user['token']) - project = env['runbot_merge.project'].search([]) b_head = prod.commit('b') c_head = prod.commit('c') with prod, other_user_repo: diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 73730d86..0ee884d2 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -326,6 +326,7 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): ] }) setreviewers(*proj.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) # create a PR in v1, merge it, then create all 3 ports with repo: diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 74862760..00dd86be 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -206,6 +206,7 @@ class TestNotAllBranches: 'branch_filter': '[("name", "in", ["a", "c"])]', }) setreviewers(repo_a, repo_b) + env['runbot_merge.events_sources'].create([{'repository': a.name}, {'repository': b.name}]) return project, a, a_dev, b, b_dev def test_single_first(self, env, repos, config): @@ -883,6 +884,7 @@ def test_disable_branch_with_batches(env, config, make_repo, users): "required_statuses": "default", "fp_remote_target": fork2.name, }) + env['runbot_merge.events_sources'].create({'repository': repo2.name}) env['res.partner'].search([ ('github_login', '=', config['role_reviewer']['user']) ]).write({ @@ -978,6 +980,7 @@ def test_disable_multitudes(env, config, make_repo, users, setreviewers): })], }) setreviewers(project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) with repo: [a, b, c, d] = repo.make_commits( diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 0d0f7dff..bc451293 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -130,6 +130,7 @@ def make_basic( }) prod = make_repo(reponame) + env['runbot_merge.events_sources'].create({'repository': prod.name}) with prod: a_0, a_1, a_2, a_3, a_4, = prod.make_commits( None, diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index b005c5a9..5f23262c 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -1,6 +1,6 @@ { 'name': 'merge bot', - 'version': '1.13', + 'version': '1.14', 'depends': ['contacts', 'mail', 'website'], 'data': [ 'security/security.xml', diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 5485f1fa..bdbd96c8 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -86,18 +86,18 @@ class MergebotController(Controller): github._gh.info(self._format(req)) - c = EVENTS.get(event) - if not c: - _logger.warning('Unknown event %s', event) - return 'Unknown event {}'.format(event) - - repo = request.jsonrequest['repository']['full_name'] + repo = request.jsonrequest.get('repository', {}).get('full_name') env = request.env(user=1) - secret = env['runbot_merge.repository'].search([ - ('name', '=', repo), - ]).project_id.secret - if secret and secret.strip(): + source = repo and env['runbot_merge.events_sources'].search([('repository', '=', repo)]) + if not source: + _logger.warning( + "Ignored hook %s to unknown source repository %s", + req.headers.get("X-Github-Delivery"), + repo, + ) + return werkzeug.exceptions.Forbidden() + elif secret := source.secret: signature = 'sha256=' + hmac.new(secret.strip().encode(), req.get_data(), hashlib.sha256).hexdigest() if not hmac.compare_digest(signature, req.headers.get('X-Hub-Signature-256', '')): _logger.warning( @@ -114,6 +114,11 @@ class MergebotController(Controller): else: _logger.info("No secret or signature for %s", repo) + c = EVENTS.get(event) + if not c: + _logger.warning('Unknown event %s', event) + return 'Unknown event {}'.format(event) + sentry_sdk.set_context('webhook', request.jsonrequest) return c(env, request.jsonrequest) diff --git a/runbot_merge/migrations/15.0.1.14/pre-migration.py b/runbot_merge/migrations/15.0.1.14/pre-migration.py new file mode 100644 index 00000000..6ac9ed1f --- /dev/null +++ b/runbot_merge/migrations/15.0.1.14/pre-migration.py @@ -0,0 +1,12 @@ +def migrate(cr, version): + cr.execute(""" + CREATE TABLE runbot_merge_events_sources ( + id serial primary key, + repository varchar not null, + secret varchar + ); + INSERT INTO runbot_merge_events_sources (repository, secret) + SELECT r.name, p.secret + FROM runbot_merge_repository r + JOIN runbot_merge_project p ON p.id = r.project_id; + """) diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py index 18367537..f15f98f2 100644 --- a/runbot_merge/models/__init__.py +++ b/runbot_merge/models/__init__.py @@ -6,4 +6,5 @@ from . import batch from . import project_freeze from . import stagings_create from . import staging_cancel +from . import events_sources from . import crons diff --git a/runbot_merge/models/events_sources.py b/runbot_merge/models/events_sources.py new file mode 100644 index 00000000..34db75ac --- /dev/null +++ b/runbot_merge/models/events_sources.py @@ -0,0 +1,12 @@ +from odoo import models, fields + + +class EventsSources(models.Model): + _name = 'runbot_merge.events_sources' + _description = 'Valid Webhook Event Sources' + _order = "repository" + _rec_name = "repository" + + # FIXME: unique repo? Or allow multiple secrets per repo? + repository = fields.Char(index=True, required=True) + secret = fields.Char() diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index db41758b..55210ebb 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -55,12 +55,6 @@ class Project(models.Model): batch_limit = fields.Integer( default=8, group_operator=None, 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." - ) - freeze_id = fields.Many2one('runbot_merge.project.freeze', compute='_compute_freeze') freeze_reminder = fields.Text() diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 28f2bd7e..9d2da846 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -21,6 +21,7 @@ access_runbot_merge_fetch_job_admin,Admin access to fetch jobs,model_runbot_merg 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 access_runbot_merge_review_override,Admin access to override permissions,model_res_partner_override,runbot_merge.group_admin,1,1,1,1 +access_runbot_merge_events_sources,Admin access to event sources,model_runbot_merge_events_sources,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_project,User access to project,model_runbot_merge_project,base.group_user,1,0,0,0 access_runbot_merge_repository,User access to repo,model_runbot_merge_repository,base.group_user,1,0,0,0 access_runbot_merge_branch,User access to branches,model_runbot_merge_branch,base.group_user,1,0,0,0 @@ -29,3 +30,4 @@ access_runbot_merge_pull_requests_feedback,Users have no reason to access feedba access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_review,base.group_user,1,0,0,0 access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0 runbot_merge.access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0 + diff --git a/runbot_merge/tests/conftest.py b/runbot_merge/tests/conftest.py index 75cad7a8..f75447ca 100644 --- a/runbot_merge/tests/conftest.py +++ b/runbot_merge/tests/conftest.py @@ -39,3 +39,30 @@ def project(env, config): 'github_prefix': 'hansen', 'branch_ids': [(0, 0, {'name': 'master'})], }) + + +@pytest.fixture +def make_repo2(env, project, make_repo, users, setreviewers): + """Layer over ``make_repo`` which also: + + - adds the new repo to ``project`` (with no group and the ``'default'`` status required) + - sets the standard reviewers on the repo + - and creates an event source for the repo + """ + def mr(name): + r = make_repo(name) + rr = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': r.name, + 'group_id': False, + 'required_statuses': 'default', + }) + setreviewers(rr) + env['runbot_merge.events_sources'].create({'repository': r.name}) + return r + return mr + + +@pytest.fixture +def repo(make_repo2): + return make_repo2('repo') diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index d43368e9..125d1477 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -12,17 +12,9 @@ from lxml import html import odoo from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one - -@pytest.fixture -def repo(env, project, make_repo, users, setreviewers): - r = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': r.name, - 'group_id': False, - 'required_statuses': 'legal/cla,ci/runbot' - })]}) - setreviewers(*project.repo_ids) - return r +@pytest.fixture(autouse=True) +def _configure_statuses(project, repo): + project.repo_ids.required_statuses = 'legal/cla,ci/runbot' @pytest.fixture(autouse=True, params=["statuses", "rpc"]) def stagings(request, env, project, repo): @@ -353,11 +345,15 @@ Co-authored-by: Bob """.format( ) class TestWebhookSecurity: + @pytest.fixture(autouse=True) + def add_secret_to_source(self, env, repo): + env['runbot_merge.events_sources'].search([ + ('repository', '=', repo.name), + ]).secret = "a secret" + def test_no_secret(self, env, project, repo): """ Test 1: didn't add a secret to the repo, should be ignored """ - project.secret = "a secret" - with repo: m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) repo.make_ref('heads/master', m) @@ -371,7 +367,6 @@ class TestWebhookSecurity: ]) def test_wrong_secret(self, env, project, repo): - project.secret = "a secret" with repo: repo.set_secret("wrong secret") @@ -387,7 +382,6 @@ class TestWebhookSecurity: ]) def test_correct_secret(self, env, project, repo): - project.secret = "a secret" with repo: repo.set_secret("a secret") @@ -2789,7 +2783,7 @@ class TestBatching(object): def test_batching_pressing(self, env, repo, config): """ "Pressing" PRs should be selected before normal & batched together """ - # by limiting the batch size to 3 we allow both high-priority PRs, but + # by limiting the batch size to 3 we allow both high-priority PRs, but # a single normal priority one env['runbot_merge.project'].search([]).batch_limit = 3 with repo: diff --git a/runbot_merge/tests/test_batch_consistency.py b/runbot_merge/tests/test_batch_consistency.py index ef3c88f6..4a35389b 100644 --- a/runbot_merge/tests/test_batch_consistency.py +++ b/runbot_merge/tests/test_batch_consistency.py @@ -4,19 +4,10 @@ without wider relevance and thus other location. from utils import Commit, to_pr -def test_close_single(env, project, make_repo, setreviewers): +def test_close_single(env, repo): """If a batch has a single PR and that PR gets closed, the batch should be inactive *and* blocked. """ - repo = make_repo('wheee') - r = env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'required_statuses': 'default', - 'group_id': False, - }) - setreviewers(r) - with repo: repo.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master') [c] = repo.make_commits('master', Commit('b', tree={"b": "b"})) @@ -41,25 +32,13 @@ def test_close_single(env, project, make_repo, setreviewers): assert Batches.search_count([]) == 0 -def test_close_multiple(env, project, make_repo, setreviewers): +def test_close_multiple(env, make_repo2): """If a batch has a single PR and that PR gets closed, the batch should be inactive *and* blocked. """ Batches = env['runbot_merge.batch'] - repo1 = make_repo('wheee') - repo2 = make_repo('wheeee') - project.write({ - 'repo_ids': [(0, 0, { - 'name': repo1.name, - 'required_statuses': 'default', - 'group_id': False, - }), (0, 0, { - 'name': repo2.name, - 'required_statuses': 'default', - 'group_id': False, - })] - }) - setreviewers(*project.repo_ids) + repo1 = make_repo2('wheee') + repo2 = make_repo2('wheeee') with repo1: repo1.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master') diff --git a/runbot_merge/tests/test_by_branch.py b/runbot_merge/tests/test_by_branch.py index 4122bf82..efaf0394 100644 --- a/runbot_merge/tests/test_by_branch.py +++ b/runbot_merge/tests/test_by_branch.py @@ -2,28 +2,21 @@ import pytest from utils import Commit - @pytest.fixture -def repo(env, project, make_repo, users, setreviewers): - r = make_repo('repo') - project.write({ - 'repo_ids': [(0, 0, { - 'name': r.name, - 'status_ids': [ - (0, 0, {'context': 'ci'}), - # require the lint status on master - (0, 0, { - 'context': 'lint', - 'branch_filter': [('id', '=', project.branch_ids.id)] - }), - (0, 0, {'context': 'pr', 'stagings': False}), - (0, 0, {'context': 'staging', 'prs': False}), - ] - })], - }) - setreviewers(*project.repo_ids) - return r +def _setup_statuses(project, repo): + project.repo_ids.status_ids = [ + (5, 0, 0), + (0, 0, {'context': 'ci'}), + # require the lint status on master + (0, 0, { + 'context': 'lint', + 'branch_filter': [('id', '=', project.branch_ids.id)] + }), + (0, 0, {'context': 'pr', 'stagings': False}), + (0, 0, {'context': 'staging', 'prs': False}), + ] +@pytest.mark.usefixtures('_setup_statuses') def test_status_applies(env, repo, config): """ If branches are associated with a repo status, only those branch should require the status on their PRs & stagings @@ -71,6 +64,7 @@ def test_status_applies(env, repo, config): env.run_crons('runbot_merge.process_updated_commits') assert st.state == 'success' +@pytest.mark.usefixtures('_setup_statuses') def test_status_skipped(env, project, repo, config): """ Branches not associated with a repo status should not require the status on their PRs or stagings @@ -132,6 +126,7 @@ def test_pseudo_version_tag(env, project, make_repo, setreviewers, config): ], }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) with repo: [m] = repo.make_commits(None, Commit('c1', tree={'a': '1'}), ref='heads/master') diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index ed970974..cfd16bae 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -23,6 +23,7 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf 'group_id': False, }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) with repo: [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') @@ -107,6 +108,7 @@ def test_new_pr_no_branch(env, project, make_repo, setreviewers, users): 'status_ids': [(0, 0, {'context': 'status'})] }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) with repo: [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') @@ -140,6 +142,7 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): 'active': False, }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) with repo: [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 42043f78..e6073028 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -19,39 +19,42 @@ from utils import seen, get_partner, pr_page, to_pr, Commit @pytest.fixture -def repo_a(project, make_repo, setreviewers): +def repo_a(env, project, make_repo, setreviewers): repo = make_repo('a') - r = project.env['runbot_merge.repository'].create({ + r = env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, 'required_statuses': 'default', 'group_id': False, }) setreviewers(r) + env['runbot_merge.events_sources'].create({'repository': r.name}) return repo @pytest.fixture -def repo_b(project, make_repo, setreviewers): +def repo_b(env, project, make_repo, setreviewers): repo = make_repo('b') - r = project.env['runbot_merge.repository'].create({ + r = env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, 'required_statuses': 'default', 'group_id': False, }) setreviewers(r) + env['runbot_merge.events_sources'].create({'repository': r.name}) return repo @pytest.fixture -def repo_c(project, make_repo, setreviewers): +def repo_c(env, project, make_repo, setreviewers): repo = make_repo('c') - r = project.env['runbot_merge.repository'].create({ + r = env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, 'required_statuses': 'default', 'group_id': False, }) setreviewers(r) + env['runbot_merge.events_sources'].create({'repository': r.name}) return repo def make_pr(repo, prefix, trees, *, target='master', user, @@ -934,6 +937,7 @@ class TestSubstitutions: 'repo_ids': [(0, 0, {'name': 'xxx/xxx'})], 'branch_ids': [(0, 0, {'name': 'master'})] }) + env['runbot_merge.events_sources'].create({'repository': 'xxx/xxx'}) r = p.repo_ids # replacement pattern, pr label, stored label cases = [ @@ -1001,8 +1005,7 @@ class TestSubstitutions: with repo_a: repo_a.make_commits('master', repo_a.Commit('bop', tree={'a': '1'}), ref='heads/abranch') pra = repo_a.make_pr(target='master', head='abranch') - b_fork = repo_b.fork() - with b_fork, repo_b: + with repo_b, repo_b.fork() as b_fork: b_fork.make_commits('master', b_fork.Commit('pob', tree={'b': '1'}), ref='heads/abranch') prb = repo_b.make_pr( title="a pr", @@ -1058,6 +1061,7 @@ def test_multi_project(env, make_repo, setreviewers, users, config, 'branch_ids': [(0, 0, {'name': 'default'})], }) setreviewers(*p1.repo_ids) + env['runbot_merge.events_sources'].create([{'repository': r1.name}]) r2 = make_repo('repo_b') with r2: @@ -1078,6 +1082,7 @@ def test_multi_project(env, make_repo, setreviewers, users, config, 'branch_ids': [(0, 0, {'name': 'default'})], }) setreviewers(*p2.repo_ids) + env['runbot_merge.events_sources'].create([{'repository': r2.name}]) assert r1_dev.owner == r2_dev.owner diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index b5d9d5b2..8b2bba21 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -96,7 +96,7 @@ def test_unreviewer(env, project, port): assert p.review_rights == env['res.partner.review'] -def test_staging_post_update(env, project, make_repo, setreviewers, users, config): +def test_staging_post_update(env, repo, users, config): """Because statuses come from commits, it's possible to update the commits of a staging after that staging has completed (one way or the other), either by sending statuses directly (e.g. rebuilding, for non-deterministic errors) @@ -105,21 +105,13 @@ def test_staging_post_update(env, project, make_repo, setreviewers, users, confi This makes post-mortem analysis quite confusing, so stagings should "lock in" their statuses once they complete. """ - repo = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': repo.name, - 'group_id': False, - 'required_statuses': 'legal/cla,ci/runbot' - })]}) - setreviewers(*project.repo_ids) with repo: [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref='heads/master') repo.make_commits(m, Commit('thing', tree={'m': 'c'}), ref='heads/other') pr = repo.make_pr(target='master', head='other') - repo.post_status(pr.head, 'success', 'ci/runbot') - repo.post_status(pr.head, 'success', 'legal/cla') + repo.post_status(pr.head, 'success') pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) env.run_crons() pr_id = to_pr(env, pr) @@ -128,33 +120,25 @@ def test_staging_post_update(env, project, make_repo, setreviewers, users, confi staging_head = repo.commit('staging.master') with repo: - repo.post_status(staging_head, 'failure', 'ci/runbot') + repo.post_status(staging_head, 'failure') env.run_crons() assert pr_id.state == 'error' assert staging_id.state == 'failure' assert staging_id.statuses == [ - [repo.name, 'ci/runbot', 'failure', ''], + [repo.name, 'default', 'failure', ''], ] with repo: - repo.post_status(staging_head, 'success', 'ci/runbot') + repo.post_status(staging_head, 'success') env.run_crons() assert staging_id.state == 'failure' assert staging_id.statuses == [ - [repo.name, 'ci/runbot', 'failure', ''], + [repo.name, 'default', 'failure', ''], ] -def test_merge_empty_commits(env, project, make_repo, setreviewers, users, config): +def test_merge_empty_commits(env, repo, users, config): """The mergebot should allow merging already-empty commits. """ - repo = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': repo.name, - 'group_id': False, - 'required_statuses': 'default', - })]}) - setreviewers(*project.repo_ids) - with repo: [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref='heads/master') @@ -187,18 +171,10 @@ def test_merge_empty_commits(env, project, make_repo, setreviewers, users, confi assert commits[1]['commit']['message'].startswith('thing1') assert commits[2]['commit']['message'] == 'initial' -def test_merge_emptying_commits(env, project, make_repo, setreviewers, users, config): +def test_merge_emptying_commits(env, repo, users, config): """The mergebot should *not* allow merging non-empty commits which become empty as part of the staging (rebasing) """ - repo = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': repo.name, - 'group_id': False, - 'required_statuses': 'default', - })]}) - setreviewers(*project.repo_ids) - with repo: [m, _] = repo.make_commits( None, @@ -253,15 +229,7 @@ def test_merge_emptying_commits(env, project, make_repo, setreviewers, users, co (users['user'], f"{ping} unable to stage: results in an empty tree when merged, might be the duplicate of a merged PR.") ] -def test_force_ready(env, make_repo, project, setreviewers, config): - repo = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': repo.name, - 'group_id': False, - 'required_statuses': 'default', - })]}) - setreviewers(*project.repo_ids) - +def test_force_ready(env, repo, config): with repo: [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master") diff --git a/runbot_merge/tests/test_project_toggles.py b/runbot_merge/tests/test_project_toggles.py index 876c241e..615e6e4b 100644 --- a/runbot_merge/tests/test_project_toggles.py +++ b/runbot_merge/tests/test_project_toggles.py @@ -6,17 +6,6 @@ import pytest from utils import Commit, to_pr, ensure_one -@pytest.fixture -def repo(env, project, make_repo, users, setreviewers): - r = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': r.name, - 'group_id': False, - 'required_statuses': 'default', - })]}) - setreviewers(*project.repo_ids) - return r - def test_disable_staging(env, project, repo, config): """In order to avoid issues of cron locking, as well as not disable staging for every project when trying to freeze just one of them (cough cough), a diff --git a/runbot_merge/tests/test_staging.py b/runbot_merge/tests/test_staging.py index 8903df62..c1aeeeeb 100644 --- a/runbot_merge/tests/test_staging.py +++ b/runbot_merge/tests/test_staging.py @@ -1,19 +1,6 @@ -import pytest - from utils import Commit, to_pr -@pytest.fixture -def repo(env, project, make_repo, users, setreviewers): - r = make_repo('repo') - project.write({'repo_ids': [(0, 0, { - 'name': r.name, - 'group_id': False, - 'required_statuses': 'default' - })]}) - setreviewers(*project.repo_ids) - return r - def test_staging_disabled_branch(env, project, repo, config): """Check that it's possible to disable staging on a specific branch """ diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py index 1bdc4877..0d13ac61 100644 --- a/runbot_merge/tests/test_status_overrides.py +++ b/runbot_merge/tests/test_status_overrides.py @@ -50,6 +50,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config): 'status_ids': [(0, 0, {'context': 'l/int'})] }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) # "other" can override the lint env['res.partner'].create({ 'name': config['role_other'].get('name', 'Other'), @@ -110,6 +111,7 @@ def test_multiple(env, project, make_repo, users, setreviewers, config): 'status_ids': [(0, 0, {'context': 'l/int'}), (0, 0, {'context': 'c/i'})] }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) # "other" can override the lints env['res.partner'].create({ 'name': config['role_other'].get('name', 'Other'), @@ -174,6 +176,7 @@ def test_no_repository(env, project, make_repo, users, setreviewers, config): 'status_ids': [(0, 0, {'context': 'l/int'})] }) setreviewers(*project.repo_ids) + env['runbot_merge.events_sources'].create({'repository': repo.name}) # "other" can override the lint env['res.partner'].create({ 'name': config['role_other'].get('name', 'Other'), diff --git a/runbot_merge/views/configuration.xml b/runbot_merge/views/configuration.xml index 32d3f3e7..0273a53a 100644 --- a/runbot_merge/views/configuration.xml +++ b/runbot_merge/views/configuration.xml @@ -63,6 +63,21 @@ + + Events Sources + runbot_merge.events_sources + + + Events Sources List + runbot_merge.events_sources + + + + + + + + @@ -70,5 +85,7 @@ action="action_review"/> + diff --git a/runbot_merge/views/runbot_merge_project.xml b/runbot_merge/views/runbot_merge_project.xml index 68eb28cc..6e83376f 100644 --- a/runbot_merge/views/runbot_merge_project.xml +++ b/runbot_merge/views/runbot_merge_project.xml @@ -29,7 +29,6 @@ help="Identity when creating new commits, defaults to github name, falls back to login."/> -