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=[])