diff --git a/conftest.py b/conftest.py index 20678ef3..72e62618 100644 --- a/conftest.py +++ b/conftest.py @@ -783,7 +783,8 @@ class Repo: def post_status(self, ref, status, context='default', **kw): assert self.hook assert status in ('error', 'failure', 'pending', 'success') - r = self._session.post('https://api.github.com/repos/{}/statuses/{}'.format(self.name, self.commit(ref).id), json={ + commit = ref if isinstance(ref, Commit) else self.commit(ref) + r = self._session.post('https://api.github.com/repos/{}/statuses/{}'.format(self.name, commit.id), json={ 'state': status, 'context': context, **kw diff --git a/runbot_merge/changelog/2022-10/statuses.md b/runbot_merge/changelog/2022-10/statuses.md new file mode 100644 index 00000000..0cc418c7 --- /dev/null +++ b/runbot_merge/changelog/2022-10/statuses.md @@ -0,0 +1,10 @@ +FIX: lock in statuses at the end of a staging + +The statuses of a staging are computed dynamically. Because github associates +statuses with *commits*, rebuilding a staging (partially or completely) or using +one of its commits for a branch could lead to the statuses becoming inconsistent +with the staging e.g. all-green statuses while the staging had failed. + +By locking in the status at the end of the staging, the dashboard is less +confusing and more consistent, and post-mortem analysis (e.g. of staging +failures) easier. diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 0cedc5fc..8134ea65 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1714,6 +1714,25 @@ class Stagings(models.Model): head_ids = fields.Many2many('runbot_merge.commit', compute='_compute_statuses') statuses = fields.Binary(compute='_compute_statuses') + statuses_cache = fields.Text() + + def write(self, vals): + # don't allow updating the statuses_cache + vals.pop('statuses_cache', None) + + if 'state' not in vals: + return super().write(vals) + + previously_pending = self.filtered(lambda s: s.state == 'pending') + super(Stagings, self).write(vals) + for staging in previously_pending: + if staging.state != 'pending': + super(Stagings, staging).write({ + 'statuses_cache': json.dumps(staging.statuses) + }) + + return True + def name_get(self): return [ @@ -1738,6 +1757,10 @@ class Stagings(models.Model): if not repo.endswith('^') } commits = st.head_ids = Commits.search([('sha', 'in', list(heads.keys()))]) + if st.statuses_cache: + st.statuses = json.loads(st.statuses_cache) + continue + st.statuses = [ ( heads[commit.sha], diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 3ad1289b..cdce2db0 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -1,6 +1,6 @@ import requests -from utils import Commit, to_pr +from utils import Commit, to_pr, seen def test_partner_merge(env): @@ -129,3 +129,51 @@ def test_unreviewer(env, project, port): assert 'error' not in r.json() assert p.review_rights == env['res.partner.review'] + +def test_staging_post_update(env, project, make_repo, setreviewers, users, config): + """Because statuses come from commits, it's possible to update the commits + of a staging after that staging has completed (one way or the other), either + by sending statuses directly (e.g. rebuilding, for non-deterministic errors) + or just using the staging's head commit in a branch. + + This makes post-mortem analysis quite confusing, so stagings should + "lock in" their statuses once they complete. + """ + repo = make_repo('repo') + project.write({'repo_ids': [(0, 0, { + 'name': repo.name, + 'group_id': False, + 'required_statuses': 'legal/cla,ci/runbot' + })]}) + setreviewers(*project.repo_ids) + + with repo: + [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref='heads/master') + + repo.make_commits(m, Commit('thing', tree={'m': 'c'}), ref='heads/other') + pr = repo.make_pr(target='master', head='other') + repo.post_status(pr.head, 'success', 'ci/runbot') + repo.post_status(pr.head, 'success', 'legal/cla') + pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) + env.run_crons() + pr_id = to_pr(env, pr) + staging_id = pr_id.staging_id + assert staging_id + + staging_head = repo.commit('staging.master') + with repo: + repo.post_status(staging_head, 'failure', 'ci/runbot') + env.run_crons() + assert pr_id.state == 'error' + assert staging_id.state == 'failure' + assert staging_id.statuses == [ + [repo.name, 'ci/runbot', 'failure', ''], + ] + + with repo: + repo.post_status(staging_head, 'success', 'ci/runbot') + env.run_crons() + assert staging_id.state == 'failure' + assert staging_id.statuses == [ + [repo.name, 'ci/runbot', 'failure', ''], + ]