From af8c62e4adee83785065b3cac963a588f07e8fad Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 10 Oct 2018 10:48:42 +0200 Subject: [PATCH] [FIX] runbot_merge: better handle targets being branch-protected If a staging covers multiple repositories and there's a fast-forward issue on any but the first repo/target, runbot_merge attempted to revert the commits it had fast-forwarded on the previous repos. This doesn't work when branch-protection is active, unless runbot_merge is a repository administrator (and branch protection is not configured to apply to those): reverting is done by push-forcing the original head back onto the ref, which branch-protection unconditionally precludes. This commit does not entirely fix the race condition (it does not look like github provides any way to do that), but it should significantly reduce the race-condition window as it performs a semi-wet run of the fast-forward process on the tmp branches before actually updating the targets. That way the only remaining breakage should be when somebody pushes on repositories 1.. between the test-FF on tmp branches and the actual fast forward. While at it, updated the github API thing to *always* dump the JSON body on an error response, if the content-type is json. --- runbot_merge/github.py | 6 +---- runbot_merge/models/pull_requests.py | 30 ++++++++++++++------- runbot_merge/tests/fake_github/__init__.py | 31 +++++++++++++++++++--- runbot_merge/tests/remote.py | 10 +++++++ runbot_merge/tests/test_basic.py | 15 +++++++++++ runbot_merge/tests/test_multirepo.py | 21 +++++++++++++++ 6 files changed, 94 insertions(+), 19 deletions(-) diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 9f158482..318961cd 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -31,11 +31,7 @@ class GH(object): exc = check.get(r.status_code) if exc: raise exc(r.content) - if r.status_code == 422: - # dump & format body if it's a 422 as GH's HTTP Reason is - # completely useless (only states - # "Unprocessable Entity for URL: " which is not - # exactly great for debugging what went wrong + if r.status_code >= 400 and r.headers.get('content-type', '').startswith('application/javascript'): raise requests.HTTPError( json_.dumps(r.json(), indent=4), response=r diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index f6dd3a47..64291e82 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -74,14 +74,28 @@ class Project(models.Model): staging, staging.state ) if staging.state == 'success': - old_heads = { - n: g.head(staging.target.name) - for n, g in gh.items() - } repo_name = None staging_heads = json.loads(staging.heads) - updated = [] try: + # reverting updates doesn't work if the branches are + # protected (because a revert is basically a force + # push), instead use the tmp branch as a dry-run + tmp_target = 'tmp.' + staging.target.name + # first force-push the current targets to all tmps + for repo_name in staging_heads.keys(): + if repo_name.endswith('^'): + continue + g = gh[repo_name] + g.set_ref(tmp_target, g.head(staging.target.name)) + + # then attempt to FF the tmp to the staging + for repo_name, head in staging_heads.items(): + if repo_name.endswith('^'): + continue + gh[repo_name].fast_forward(tmp_target, staging_heads.get(repo_name + '^') or head) + + # there is still a race condition here, but it's way + # lower than "the entire staging duration"... for repo_name, head in staging_heads.items(): if repo_name.endswith('^'): continue @@ -92,16 +106,12 @@ class Project(models.Model): staging.target.name, staging_heads.get(repo_name + '^') or head ) - updated.append(repo_name) except exceptions.FastForwardError as e: logger.warning( - "Could not fast-forward successful staging on %s:%s, reverting updated repos %s and re-staging", + "Could not fast-forward successful staging on %s:%s", repo_name, staging.target.name, - ', '.join(updated), exc_info=True ) - for name in reversed(updated): - gh[name].set_ref(staging.target.name, old_heads[name]) staging.write({ 'state': 'ff_failed', 'reason': str(e.__cause__ or e.__context__ or '') diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index e6ceed53..78e2061a 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/runbot_merge/tests/fake_github/__init__.py @@ -94,6 +94,7 @@ class Repo(object): self.refs = {} # {event: (wsgi_app, url)} self.hooks = collections.defaultdict(list) + self.protected = set() def hook(self, hook, events): for event in events: @@ -118,12 +119,34 @@ class Repo(object): assert 'heads/%s' % target in self.refs return PR(self, title, body, target, ctid, user=user, label='{}:{}'.format(user, label or target)) + def get_ref(self, ref): + if re.match(r'[0-9a-f]{40}', ref): + return ref + + sha = self.refs.get(ref) + assert sha, "no ref %s" % ref + return sha + def make_ref(self, name, commit, force=False): assert isinstance(self.objects[commit], Commit) if not force and name in self.refs: raise ValueError("ref %s already exists" % name) self.refs[name] = commit + def protect(self, branch): + ref = 'heads/%s' % branch + assert ref in self.refs + self.protected.add(ref) + + def update_ref(self, name, commit, force=False): + current = self.refs.get(name) + assert current is not None + + assert name not in self.protected and force or git.is_ancestor( + self.objects, current, commit) + + self.make_ref(name, commit, force=True) + def commit(self, ref): sha = self.refs.get(ref) or ref commit = self.objects[sha] @@ -252,11 +275,11 @@ class Repo(object): if sha not in self.objects: return (404, None) - if not body.get('force'): - if not git.is_ancestor(self.objects, current, sha): - return (400, None) + try: + self.update_ref(ref, sha, body.get('force') or False) + except AssertionError: + return (400, None) - self.make_ref(ref, sha, force=True) return (200, { "ref": "refs/%s" % ref, "object": { diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index 9601382e..ba50a557 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -446,6 +446,16 @@ class Repo: assert 200 <= r.status_code < 300, r.json() wait_for_hook() + def protect(self, branch): + r = self._session.put('https://api.github.com/repos/{}/branches/{}/protection'.format(self.name, branch), json={ + 'required_status_checks': None, + 'enforce_admins': True, + 'required_pull_request_reviews': None, + 'restrictions': None, + }) + assert 200 <= r.status_code < 300, r.json() + wait_for_hook() + def update_ref(self, name, commit, force=False): r = self._session.patch('https://api.github.com/repos/{}/git/refs/{}'.format(self.name, name), json={'sha': commit, 'force': force}) assert 200 <= r.status_code < 300, r.json() diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index a9cd6fcd..4a360e03 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -5,6 +5,7 @@ import time import requests import pytest +from requests import HTTPError import odoo @@ -1913,6 +1914,20 @@ class TestComments: assert {p.github_login for p in pr.delegates} \ == {'foo', 'bar', 'baz'} +class TestInfrastructure: + def test_protection(self, repo): + """ force-pushing on a protected ref should fail + """ + m0 = repo.make_commit(None, 'initial', None, tree={'m': 'm0'}) + m1 = repo.make_commit(m0, 'first', None, tree={'m': 'm1'}) + repo.make_ref('heads/master', m1) + repo.protect('master') + + c1 = repo.make_commit(m0, 'other', None, tree={'m': 'c1'}) + with pytest.raises(AssertionError): + repo.update_ref('heads/master', c1, force=True) + assert repo.get_ref('heads/master') == m1 + def node(name, *children): assert type(name) in (str, re_matches) return name, frozenset(children) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 21e42631..5d54a332 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -64,12 +64,14 @@ def test_stage_one(env, project, repo_a, repo_b): 'heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) ) + repo_a.protect('master') pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') repo_b.make_ref( 'heads/master', repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) ) + repo_b.protect('master') pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-other-thing') env['runbot_merge.project']._check_progress() @@ -87,12 +89,14 @@ def test_stage_match(env, project, repo_a, repo_b): 'heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) ) + repo_a.protect('master') pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') repo_b.make_ref( 'heads/master', repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) ) + repo_b.protect('master') pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing') env['runbot_merge.project']._check_progress() @@ -115,18 +119,21 @@ def test_sub_match(env, project, repo_a, repo_b, repo_c): 'heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) ) + repo_a.protect('master') # no pr here repo_b.make_ref( 'heads/master', repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) ) + repo_b.protect('master') pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing') repo_c.make_ref( 'heads/master', repo_c.make_commit(None, 'initial', None, tree={'a': 'c_0'}) ) + repo_c.protect('master') pr_c = make_pr(repo_c, 'C', [{'a': 'c_1'}], label='do-a-thing') env['runbot_merge.project']._check_progress() @@ -161,8 +168,10 @@ def test_merge_fail(env, project, repo_a, repo_b, users): root_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) repo_a.make_ref('heads/master', root_a) + repo_a.protect('master') root_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) repo_b.make_ref('heads/master', root_b) + repo_b.protect('master') # first set of matched PRs pr1a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') @@ -206,10 +215,12 @@ def test_ff_fail(env, project, repo_a, repo_b): project.batch_limit = 1 root_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) repo_a.make_ref('heads/master', root_a) + repo_a.protect('master') make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') root_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) repo_b.make_ref('heads/master', root_b) + repo_b.protect('master') make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing') env['runbot_merge.project']._check_progress() @@ -241,11 +252,13 @@ def test_one_failed(env, project, repo_a, repo_b, owner): project.batch_limit = 1 c_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) repo_a.make_ref('heads/master', c_a) + repo_a.protect('master') # pr_a is born ready pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') c_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) repo_b.make_ref('heads/master', c_b) + repo_b.protect('master') c_pr = repo_b.make_commit(c_b, 'pr', None, tree={'a': 'b_1'}) pr_b = repo_b.make_pr( 'title', 'body', target='master', ctid=c_pr, @@ -273,6 +286,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users): """ c_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) repo_a.make_ref('heads/master', c_a) + repo_a.protect('master') # pr_a is born ready pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') repo_a.post_status(pr_a.head, 'success', 'ci/runbot') @@ -280,6 +294,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users): c_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) repo_b.make_ref('heads/master', c_b) + repo_b.protect('master') env['runbot_merge.project']._check_progress() pr = to_pr(env, pr_a) @@ -305,7 +320,9 @@ def test_batching(env, project, repo_a, repo_b): """ project.batch_limit = 3 repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a0'})) + repo_a.protect('master') repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b': 'b0'})) + repo_b.protect('master') prs = [( a and to_pr(env, make_pr(repo_a, 'A{}'.format(i), [{'a{}'.format(i): 'a{}'.format(i)}], label='batch{}'.format(i))), @@ -334,7 +351,9 @@ def test_batching_split(env, repo_a, repo_b): """ If a staging fails, it should get split properly across repos """ repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a0'})) + repo_a.protect('master') repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b': 'b0'})) + repo_b.protect('master') prs = [( a and to_pr(env, make_pr(repo_a, 'A{}'.format(i), [{'a{}'.format(i): 'a{}'.format(i)}], label='batch{}'.format(i))), @@ -377,7 +396,9 @@ def test_urgent(env, repo_a, repo_b): being prioritized """ repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a0': 'a'})) + repo_a.protect('master') repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b0': 'b'})) + repo_b.protect('master') pr_a = make_pr(repo_a, 'A', [{'a1': 'a'}, {'a2': 'a'}], label='batch', reviewer=None, statuses=[]) pr_b = make_pr(repo_b, 'B', [{'b1': 'b'}, {'b2': 'b'}], label='batch', reviewer=None, statuses=[])