From 9d661480fcb4d6733035f717f55c0a65cf15cca1 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 7 Feb 2020 11:52:42 +0100 Subject: [PATCH] [IMP] runbot_merge: split staging cron in two The staging cron was already essentially split between "check if one of the stagings is successful (and merge it)" and "check if we should create a staging" as these were two separate loops in the cron. But it might be useful to disable these two operations separately e.g. we might want to stop the creation of new staging but let the existing stagings complete. The actual splitting is easy but it turns out a bunch of tests were "optimised" to only run the merge cron. Most of them didn't blow up but it seems more prudent to fix them all. fixes odoo/runbot#310 --- forwardport/tests/conftest.py | 1 + runbot_merge/data/merge_cron.xml | 14 ++++++++++++-- runbot_merge/models/pull_requests.py | 23 +++++++++++------------ runbot_merge/tests/conftest.py | 4 +++- runbot_merge/tests/test_basic.py | 12 ++++++------ runbot_merge/tests/test_multirepo.py | 2 +- 6 files changed, 34 insertions(+), 22 deletions(-) diff --git a/forwardport/tests/conftest.py b/forwardport/tests/conftest.py index 3533de46..b8e530bc 100644 --- a/forwardport/tests/conftest.py +++ b/forwardport/tests/conftest.py @@ -13,6 +13,7 @@ def default_crons(): return [ 'runbot_merge.process_updated_commits', 'runbot_merge.merge_cron', + 'runbot_merge.staging_cron', 'forwardport.port_forward', 'forwardport.updates', 'runbot_merge.check_linked_prs_status', diff --git a/runbot_merge/data/merge_cron.xml b/runbot_merge/data/merge_cron.xml index 36c5dcc6..cac69dfd 100644 --- a/runbot_merge/data/merge_cron.xml +++ b/runbot_merge/data/merge_cron.xml @@ -1,9 +1,19 @@ - Check for progress of PRs & Stagings + Check for progress of (and merge) stagings code - model._check_progress(True) + model._check_stagings(True) + 1 + minutes + -1 + + + + Check for progress of PRs and create Stagings + + code + model._create_stagings(True) 1 minutes -1 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index ee4bef8d..f18609a6 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -64,23 +64,22 @@ class Project(models.Model): "will lead to webhook rejection. Should only use ASCII." ) - def _check_progress(self, commit=False): - for project in self.search([]): - for staging in project.mapped('branch_ids.active_staging_id'): - staging.check_status() - if commit: - self.env.cr.commit() + def _create_stagings(self, commit=False): + pass - for branch in project.branch_ids: + def _check_stagings(self, commit=False): + for staging in self.search([]).mapped('branch_ids.active_staging_id'): + staging.check_status() + if commit: + self.env.cr.commit() + + def _create_stagings(self, commit=False): + for branch in self.search([]).mapped('branch_ids'): + if not branch.active_staging_id: branch.try_staging() if commit: self.env.cr.commit() - # I have no idea why this is necessary for tests to pass, the only - # DB update done not through the ORM is when receiving a notification - # that a PR has been closed - self.invalidate_cache() - def _send_feedback(self): Repos = self.env['runbot_merge.repository'] ghs = {} diff --git a/runbot_merge/tests/conftest.py b/runbot_merge/tests/conftest.py index e16707d2..53e5ab2b 100644 --- a/runbot_merge/tests/conftest.py +++ b/runbot_merge/tests/conftest.py @@ -21,8 +21,10 @@ def default_crons(): 'runbot_merge.fetch_prs_cron', # env['runbot_merge.commit']._notify() 'runbot_merge.process_updated_commits', - # env['runbot_merge.project']._check_progress() + # env['runbot_merge.project']._check_stagings() 'runbot_merge.merge_cron', + # env['runbot_merge.project']._create_stagings() + '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() diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 7f0d4827..ebe2e8d9 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -464,7 +464,7 @@ def test_staging_ci_timeout(env, repo, config): timeout = env['runbot_merge.project'].search([]).ci_timeout pr1.staging_id.staged_at = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(minutes=2*timeout)) - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') assert pr1.state == 'error', "timeout should fail the PR" def test_timeout_bump_on_pending(env, repo, config): @@ -1068,7 +1068,7 @@ class TestRetry: ('repository.name', '=', repo.name), ('number', '=', prx.number) ]).state == 'ready' - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') staging_head2 = repo.commit('heads/staging.master') assert staging_head2 != staging_head @@ -1496,7 +1496,7 @@ class TestMergeMethod: c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'}) prx = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c0) - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') with repo: repo.post_status(prx.head, 'success', 'legal/cla') @@ -1538,7 +1538,7 @@ class TestMergeMethod: c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'}) prx = repo.make_pr(title="gibberish", body=None, target='master', head=c0) - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') with repo: repo.post_status(prx.head, 'success', 'legal/cla') @@ -2379,7 +2379,7 @@ class TestBatching(object): with repo: repo.post_status(h, 'success', 'ci/runbot') repo.post_status(h, 'success', 'legal/cla') - env.run_crons('runbot_merge.process_updated_commits', 'runbot_merge.merge_cron') + env.run_crons('runbot_merge.process_updated_commits', 'runbot_merge.merge_cron', 'runbot_merge.staging_cron') assert pr2.state == 'merged' class TestReviewing(object): @@ -2630,7 +2630,7 @@ class TestUnknownPR: ]) assert pr.state == 'ready' - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') assert pr.staging_id def test_rplus_unmanaged(self, env, repo, users, config): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index a92b3ea4..0aceee03 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -366,7 +366,7 @@ def test_ff_fail(env, project, repo_a, repo_b, config): repo_a.post_status('heads/staging.master', 'success', 'legal/cla') repo_b.post_status('heads/staging.master', 'success', 'ci/runbot') repo_b.post_status('heads/staging.master', 'success', 'legal/cla') - env.run_crons('runbot_merge.merge_cron') + env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') assert repo_b.commit('heads/master').id == cn,\ "B should still be at the conflicting commit" assert repo_a.commit('heads/master').id == root_a,\