[FIX] runbot_merge: stagings involving repos w/o required statuses

PRs transitioning to 'ready' had been checked and tested but turns out
I had completely forgotten to test that stagings would validate
properly therefore of course they didn't.

The issue here was I'd forgotten `''.split(',')` returns `['']` rather
than `[]`, so on an empty required_statuses the staging validator
would keep looking for a status matching the context `''` and would
never find it, keeping the staging pending until timeout. So most
likely the problem could have been resolved by just adding a condition
to

    [r.strip() for r in repomap[c.sha].required_statuses.split(',')]

but I'd already done all the rest of the reorganisation by that point,
test pass and I think it's a somewhat better logic. Therefore I'll go
with that for now.

* properly handle empty required_statuses during staging validation
* remove the final postcondition, if we're missing commits which don't
  require any statuse we should not care
* expand test to include up to merging PRs
* automatically create dummy commits when creating stagings, that way
  the relevant commits are in the database (can't hurt)

PS: an other alternative would have been to filter out or skip ahead
on commits which don't require any statuses aka cmap &
required_statuse / cmap would not even have that entry
This commit is contained in:
Xavier Morel 2020-02-07 08:22:52 +01:00
parent fd24b791b3
commit 4bdf7e5eda
2 changed files with 24 additions and 15 deletions

View File

@ -432,6 +432,10 @@ class Branch(models.Model):
# $repo is the head to check, $repo^ is the head to merge
heads[repo.name + '^'] = it['head']
heads[repo.name] = dummy_head['sha']
self.env['runbot_merge.commit'].create({
'sha': dummy_head['sha'],
'statuses': '{}',
})
# create actual staging object
st = self.env['runbot_merge.stagings'].create({
@ -1466,22 +1470,25 @@ class Stagings(models.Model):
repos = {
repo.name: repo
for repo in self.env['runbot_merge.repository'].search([])
.having_branch(s.target)
}
repomap = {
head: repos[repo]
# maps commits to the statuses they need
required_statuses = [
(head, repos[repo].required_statuses.split(','))
for repo, head in json.loads(s.heads).items()
if not repo.endswith('^')
]
# maps commits to their statuses
cmap = {
c.sha: json.loads(c.statuses)
for c in Commits.search([('sha', 'in', [h for h, _ in required_statuses])])
}
commits = Commits.search([
('sha', 'in', list(repomap))
])
update_timeout_limit = False
st = 'success'
for c in commits:
reqs = [r.strip() for r in repomap[c.sha].required_statuses.split(',')]
statuses = json.loads(c.statuses)
for v in map(lambda n: state_(statuses, n), reqs):
for head, reqs in required_statuses:
statuses = cmap.get(head) or {}
for v in map(lambda n: state_(statuses, n), filter(None, reqs)):
if st == 'failure' or v in ('error', 'failure'):
st = 'failure'
elif v is None:
@ -1491,10 +1498,6 @@ class Stagings(models.Model):
update_timeout_limit = True
else:
assert v == 'success'
# mark failure as soon as we find a failed status, but wait until
# all commits are known & not pending to mark a success
if st == 'success' and len(commits) < len(repomap):
st = 'pending'
vals = {'state': st}
if update_timeout_limit:

View File

@ -950,10 +950,16 @@ def test_no_required_statuses(env, repo, config):
prx.post_comment('hansen r+', config['role_reviewer']['token'])
env.run_crons()
assert env['runbot_merge.pull_requests'].search([
pr = env['runbot_merge.pull_requests'].search([
('repository.name', '=', repo.name),
('number', '=', prx.number)
]).state == 'ready'
])
assert pr.state == 'ready'
st = pr.staging_id
assert st
env.run_crons()
assert st.state == 'success'
assert pr.state == 'merged'
class TestRetry:
@pytest.mark.xfail(reason="This may not be a good idea as it could lead to tons of rebuild spam")