mirror of
https://github.com/odoo/runbot.git
synced 2025-03-27 13:25:47 +07:00
[FIX] runbot_merge: blocked flag false positive
The "blocked" computation would not take branch targets in account, so PRs with the same label targeting *different branches* (possible if somewhat rare due to our naming conventions) could block one another, despite really being unrelated. Also fix up some messages: * if a PR is blocked due to having no merge method, it should say that, not "has no merge" (no merge what?) * format un-managed branches as `$repo:$branch` in logging messages, `$repo#$thing` is for issues / PRs and `$branch` alone can be very unhelpful Closes #407
This commit is contained in:
parent
91de4f9314
commit
c93ce7c2c2
@ -129,7 +129,7 @@ def handle_pr(env, event):
|
|||||||
return "Nothing to update ({})".format(event['changes'].keys())
|
return "Nothing to update ({})".format(event['changes'].keys())
|
||||||
|
|
||||||
if not branch:
|
if not branch:
|
||||||
_logger.info("Ignoring PR for un-managed branch %s#%s", r, b)
|
_logger.info("Ignoring PR for un-managed branch %s:%s", r, b)
|
||||||
return "Not set up to care about {}:{}".format(r, b)
|
return "Not set up to care about {}:{}".format(r, b)
|
||||||
|
|
||||||
author_name = pr['user']['login']
|
author_name = pr['user']['login']
|
||||||
|
@ -288,7 +288,8 @@ All substitutions are tentatively applied sequentially to the input.
|
|||||||
issue, pr = gh.pr(number)
|
issue, pr = gh.pr(number)
|
||||||
|
|
||||||
if not self.project_id._has_branch(pr['base']['ref']):
|
if not self.project_id._has_branch(pr['base']['ref']):
|
||||||
_logger.info("Tasked with loading PR %d for un-managed branch %s, ignoring", pr['number'], pr['base']['ref'])
|
_logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring",
|
||||||
|
pr['number'], self.name, pr['base']['ref'])
|
||||||
self.env['runbot_merge.pull_requests.feedback'].create({
|
self.env['runbot_merge.pull_requests.feedback'].create({
|
||||||
'repository': self.id,
|
'repository': self.id,
|
||||||
'pull_request': number,
|
'pull_request': number,
|
||||||
@ -664,13 +665,14 @@ class PullRequests(models.Model):
|
|||||||
batch = pr
|
batch = pr
|
||||||
if not re.search(r':patch-\d+', pr.label):
|
if not re.search(r':patch-\d+', pr.label):
|
||||||
batch = self.search([
|
batch = self.search([
|
||||||
|
('target', '=', pr.target.id),
|
||||||
('label', '=', pr.label),
|
('label', '=', pr.label),
|
||||||
('state', 'not in', ('merged', 'closed')),
|
('state', 'not in', ('merged', 'closed')),
|
||||||
])
|
])
|
||||||
|
|
||||||
# check if PRs are configured (single commit or merge method set)
|
# check if PRs are configured (single commit or merge method set)
|
||||||
if not (pr.squash or pr.merge_method):
|
if not (pr.squash or pr.merge_method):
|
||||||
pr.blocked = 'has no merge'
|
pr.blocked = 'has no merge method'
|
||||||
continue
|
continue
|
||||||
other_unset = next((p for p in batch if not (p.squash or p.merge_method)), None)
|
other_unset = next((p for p in batch if not (p.squash or p.merge_method)), None)
|
||||||
if other_unset:
|
if other_unset:
|
||||||
|
@ -6,6 +6,7 @@ When preparing a staging, we simply want to ensure branch-matched PRs
|
|||||||
are staged concurrently in all repos
|
are staged concurrently in all repos
|
||||||
"""
|
"""
|
||||||
import json
|
import json
|
||||||
|
import time
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
import requests
|
import requests
|
||||||
@ -153,6 +154,52 @@ def test_stage_match(env, project, repo_a, repo_b, config):
|
|||||||
assert 'Related: {}'.format(pr_b.display_name) in repo_a.commit('master').message
|
assert 'Related: {}'.format(pr_b.display_name) in repo_a.commit('master').message
|
||||||
assert 'Related: {}'.format(pr_a.display_name) in repo_b.commit('master').message
|
assert 'Related: {}'.format(pr_a.display_name) in repo_b.commit('master').message
|
||||||
|
|
||||||
|
def test_different_targets(env, project, repo_a, repo_b, config):
|
||||||
|
""" PRs with different targets should not be matched together
|
||||||
|
"""
|
||||||
|
project.write({
|
||||||
|
'batch_limit': 1,
|
||||||
|
'branch_ids': [(0, 0, {'name': 'other'})]
|
||||||
|
})
|
||||||
|
with repo_a:
|
||||||
|
make_branch(repo_a, 'master', 'initial', {'master': 'a_0'})
|
||||||
|
make_branch(repo_a, 'other', 'initial', {'other': 'a_0'})
|
||||||
|
pr_a = make_pr(
|
||||||
|
repo_a, 'do-a-thing', [{'mater': 'a_1'}],
|
||||||
|
target='master',
|
||||||
|
user=config['role_user']['token'],
|
||||||
|
reviewer=config['role_reviewer']['token'],
|
||||||
|
)
|
||||||
|
with repo_b:
|
||||||
|
make_branch(repo_b, 'master', 'initial', {'master': 'b_0'})
|
||||||
|
make_branch(repo_b, 'other', 'initial', {'other': 'b_0'})
|
||||||
|
pr_b = make_pr(
|
||||||
|
repo_b, 'do-a-thing', [{'other': 'b_1'}],
|
||||||
|
target='other',
|
||||||
|
user=config['role_user']['token'],
|
||||||
|
reviewer=config['role_reviewer']['token'],
|
||||||
|
statuses=[],
|
||||||
|
)
|
||||||
|
time.sleep(5)
|
||||||
|
env.run_crons()
|
||||||
|
|
||||||
|
pr_a = to_pr(env, pr_a)
|
||||||
|
pr_b = to_pr(env, pr_b)
|
||||||
|
assert pr_a.state == 'ready'
|
||||||
|
assert not pr_a.blocked
|
||||||
|
assert pr_a.staging_id
|
||||||
|
|
||||||
|
assert pr_b.blocked
|
||||||
|
assert pr_b.state == 'approved'
|
||||||
|
assert not pr_b.staging_id
|
||||||
|
|
||||||
|
for r in [repo_a, repo_b]:
|
||||||
|
with r:
|
||||||
|
r.post_status('staging.master', 'success', 'legal/cla')
|
||||||
|
r.post_status('staging.master', 'success', 'ci/runbot')
|
||||||
|
env.run_crons()
|
||||||
|
assert pr_a.state == 'merged'
|
||||||
|
|
||||||
def test_stage_different_statuses(env, project, repo_a, repo_b, config):
|
def test_stage_different_statuses(env, project, repo_a, repo_b, config):
|
||||||
project.batch_limit = 1
|
project.batch_limit = 1
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user