mirror of
https://github.com/odoo/runbot.git
synced 2025-03-15 23:45:44 +07:00
[IMP] runbot_merge: be more optimistic about PR mergeability
If we can't stage a PR, rather than immediately put them in error wait until they were the first we tried staging, otherwise they might have been conflicting with the previous batch which ultimately won't be merged for other reason and they would have worked as part of the next batch. Note: this commit will lead to false negatives because it's batch-based rather than repo-based, so if the first batch only affects repo A and the second batch only affects repo B, if the second batch triggers a merge error it should be rejected immediately (as it's applied on a "pristine" repo B) but this change will just bump it to the next staging. fixes #209
This commit is contained in:
Xavier Morel
parent
c78ffb9e3f
commit
3b28d7801d
@ -237,7 +237,6 @@ class StatusConfiguration(models.Model):
|
||||
context = fields.Char(required=True)
|
||||
repo_id = fields.Many2one('runbot_merge.repository', required=True, ondelete='cascade')
|
||||
branch_filter = fields.Char(help="branches this status applies to")
|
||||
# FIXME: migrate branch_ids -> branch_filter = [('id', 'in', branch_ids.ids)]
|
||||
prs = fields.Boolean(string="Applies to pull requests", default=True)
|
||||
stagings = fields.Boolean(string="Applies to stagings", default=True)
|
||||
|
||||
@ -484,10 +483,25 @@ class Branch(models.Model):
|
||||
gh.set_ref('tmp.{}'.format(self.name), it['head'])
|
||||
|
||||
batch_limit = self.project_id.batch_limit
|
||||
first = True
|
||||
for batch in batched_prs:
|
||||
if len(staged) >= batch_limit:
|
||||
break
|
||||
staged |= Batch.stage(meta, batch)
|
||||
try:
|
||||
staged |= Batch.stage(meta, batch)
|
||||
except exceptions.MergeError as e:
|
||||
if first:
|
||||
pr = e.args[0]
|
||||
_logger.exception("Failed to merge %s into staging branch",
|
||||
pr.display_name)
|
||||
pr.state = 'error'
|
||||
self.env['runbot_merge.pull_requests.feedback'].create({
|
||||
'repository': pr.repository.id,
|
||||
'pull_request': pr.number,
|
||||
'message': "Unable to stage PR (%s)" % e.__context__,
|
||||
})
|
||||
else:
|
||||
first = False
|
||||
|
||||
if not staged:
|
||||
return
|
||||
@ -1925,6 +1939,14 @@ class Batch(models.Model):
|
||||
original_head, new_heads[pr]
|
||||
)
|
||||
except (exceptions.MergeError, AssertionError) as e:
|
||||
# reset the head which failed, as rebase() may have partially
|
||||
# updated it (despite later steps failing)
|
||||
gh.set_ref(target, original_head)
|
||||
# then reset every previous update
|
||||
for to_revert in new_heads.keys():
|
||||
it = meta[to_revert.repository]
|
||||
it['gh'].set_ref('tmp.{}'.format(to_revert.target.name), it['head'])
|
||||
|
||||
if isinstance(e, exceptions.Skip):
|
||||
old_head, new_head, to_squash = e.args
|
||||
pr.write({
|
||||
@ -1936,31 +1958,18 @@ class Batch(models.Model):
|
||||
"head mismatch on %s: had %s but found %s",
|
||||
pr.display_name, old_head, new_head
|
||||
)
|
||||
msg = "We apparently missed an update to this PR and" \
|
||||
" tried to stage it in a state which might not have" \
|
||||
" been approved. PR has been updated to %s, please" \
|
||||
" check and approve or re-approve." % new_head
|
||||
self.env['runbot_merge.pull_requests.feedback'].create({
|
||||
'repository': pr.repository.id,
|
||||
'pull_request': pr.number,
|
||||
'message': "We apparently missed an update to this PR "
|
||||
"and tried to stage it in a state which "
|
||||
"might not have been approved. PR has been "
|
||||
"updated to %s, please check and approve or "
|
||||
"re-approve." % new_head
|
||||
})
|
||||
return self.env['runbot_merge.batch']
|
||||
else:
|
||||
_logger.exception("Failed to merge %s into staging branch (error: %s)",
|
||||
pr.display_name, e)
|
||||
pr.state = 'error'
|
||||
msg = "Unable to stage PR (%s)" % e
|
||||
|
||||
self.env['runbot_merge.pull_requests.feedback'].create({
|
||||
'repository': pr.repository.id,
|
||||
'pull_request': pr.number,
|
||||
'message': msg,
|
||||
})
|
||||
|
||||
# reset the head which failed, as rebase() may have partially
|
||||
# updated it (despite later steps failing)
|
||||
gh.set_ref(target, original_head)
|
||||
# then reset every previous update
|
||||
for to_revert in new_heads.keys():
|
||||
it = meta[to_revert.repository]
|
||||
it['gh'].set_ref('tmp.{}'.format(to_revert.target.name), it['head'])
|
||||
|
||||
return self.env['runbot_merge.batch']
|
||||
raise exceptions.MergeError(pr)
|
||||
|
||||
# update meta to new heads
|
||||
for pr, head in new_heads.items():
|
||||
|
||||
@ -11,6 +11,8 @@ from lxml import html
|
||||
import odoo
|
||||
|
||||
from test_utils import re_matches, get_partner, _simple_init
|
||||
from utils import Commit
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def repo(env, project, make_repo, users, setreviewers):
|
||||
@ -322,7 +324,7 @@ class TestWebhookSecurity:
|
||||
('number', '=', pr0.number),
|
||||
])
|
||||
|
||||
def test_staging_conflict(env, repo, config):
|
||||
def test_staging_ongoing(env, repo, config):
|
||||
with repo:
|
||||
# create base branch
|
||||
m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
|
||||
@ -411,8 +413,9 @@ def test_staging_concurrent(env, repo, config):
|
||||
])
|
||||
assert pr2.staging_id
|
||||
|
||||
def test_staging_merge_fail(env, repo, users, config):
|
||||
""" # of staging failure (no CI) before mark & notify?
|
||||
def test_staging_confict_first(env, repo, users, config):
|
||||
""" If the first batch of a staging triggers a conflict, the PR should be
|
||||
marked as in error
|
||||
"""
|
||||
with repo:
|
||||
m1 = repo.make_commit(None, 'initial', None, tree={'f': 'm1'})
|
||||
@ -439,6 +442,52 @@ def test_staging_merge_fail(env, repo, users, config):
|
||||
(users['user'], re_matches('^Unable to stage PR')),
|
||||
]
|
||||
|
||||
def test_staging_conflict_second(env, repo, users, config):
|
||||
""" If the non-first batch of a staging triggers a conflict, the PR should
|
||||
just be skipped: it might be a conflict with an other PR which could fail
|
||||
the staging
|
||||
"""
|
||||
with repo:
|
||||
[m] = repo.make_commits(None, Commit('initial', tree={'a': '1'}), ref='heads/master')
|
||||
|
||||
with repo:
|
||||
repo.make_commits(m, Commit('first pr', tree={'a': '2'}), ref='heads/pr0')
|
||||
pr0 = repo.make_pr(target='master', head='pr0')
|
||||
repo.post_status(pr0.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr0.head, 'success', 'legal/cla')
|
||||
pr0.post_comment('hansen r+', config['role_reviewer']['token'])
|
||||
|
||||
with repo:
|
||||
repo.make_commits(m, Commit('second pr', tree={'a': '3'}), ref='heads/pr1')
|
||||
pr1 = repo.make_pr(target='master', head='pr1')
|
||||
repo.post_status(pr1.head, 'success', 'ci/runbot')
|
||||
repo.post_status(pr1.head, 'success', 'legal/cla')
|
||||
pr1.post_comment('hansen r+', config['role_reviewer']['token'])
|
||||
|
||||
env.run_crons()
|
||||
PRs = env['runbot_merge.pull_requests']
|
||||
pr0_id = PRs.search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', pr0.number),
|
||||
])
|
||||
pr1_id = PRs.search([
|
||||
('repository.name', '=', repo.name),
|
||||
('number', '=', pr1.number),
|
||||
])
|
||||
assert pr0_id.staging_id, "pr0 should have been staged"
|
||||
assert not pr1_id.staging_id, "pr1 should not have been staged (due to conflict)"
|
||||
assert pr1_id.state == 'ready', "pr1 should not be in error yet"
|
||||
|
||||
# merge the staging, this should try to stage pr1, fail, and put it in error
|
||||
# as it now conflicts with the master proper
|
||||
with repo:
|
||||
repo.post_status('staging.master', 'success', 'ci/runbot')
|
||||
repo.post_status('staging.master', 'success', 'legal/cla')
|
||||
env.run_crons()
|
||||
|
||||
assert pr1_id.state == 'error', "now pr1 should be in error"
|
||||
|
||||
|
||||
def test_staging_ci_timeout(env, repo, config):
|
||||
"""If a staging timeouts (~ delay since staged greater than
|
||||
configured)... requeue?
|
||||
|
||||
Loading…
Reference in New Issue
Block a user