From 429257d013dc5e5f2272f4be17b28832501d3488 Mon Sep 17 00:00:00 2001
From: xmo-odoo <xmo@odoo.com>
Date: Wed, 21 Aug 2019 11:17:04 +0200
Subject: [PATCH] [FIX] runbot_merge: resync tags on stage change

Before this change mergebot assumes github's tags are in sync with its
"previous" state, but because tags update was highly non-atomic (one
call per removal plus one for additions) and state can further change
between a failure and an update retry (especially as the labels endpoint
fails *a lot*), it's possible for set tags (in github) to be completely
desync'd from the mergebot state, leading to very misleading on-pr
indications.

This first fetches the current tagstate from github (to not lose non-
mergebot tags) then (hopefully atomically) resets all tags tags based on
the current mergebot state. This should avoid desyncs, and eventually
resync PRs (if they change state).

Fixes #170
---
 runbot_merge/github.py                     | 23 ++++-----
 runbot_merge/models/pull_requests.py       |  5 +-
 runbot_merge/tests/fake_github/__init__.py | 29 +++++------
 runbot_merge/tests/remote.py               | 59 +++++++++++++++++++---
 runbot_merge/tests/test_basic.py           | 54 ++++++++++++++++++++
 5 files changed, 131 insertions(+), 39 deletions(-)

diff --git a/runbot_merge/github.py b/runbot_merge/github.py
index 0f53a286..147ad80a 100644
--- a/runbot_merge/github.py
+++ b/runbot_merge/github.py
@@ -18,6 +18,7 @@ class GH(object):
         self._repo = repo
         session = self._session = requests.Session()
         session.headers['Authorization'] = 'token {}'.format(token)
+        session.headers['Accept'] = 'application/vnd.github.symmetra-preview+json'
 
     def __call__(self, method, path, params=None, json=None, check=True):
         """
@@ -97,19 +98,17 @@ class GH(object):
         self.comment(pr, message)
         self('PATCH', 'pulls/{}'.format(pr), json={'state': 'closed'})
 
-    def change_tags(self, pr, from_, to_):
-        to_add, to_remove = to_ - from_, from_ - to_
-        for t in to_remove:
-            r = self('DELETE', 'issues/{}/labels/{}'.format(pr, t), check=False)
-            # successful deletion or attempt to delete a tag which isn't there
-            # is fine, otherwise trigger an error
-            if r.status_code not in (200, 404):
-                r.raise_for_status()
+    def change_tags(self, pr, to_):
+        labels_endpoint = 'issues/{}/labels'.format(pr)
+        from .models.pull_requests import _TAGS
+        mergebot_tags = set.union(*_TAGS.values())
+        tags_before = {label['name'] for label in self('GET', labels_endpoint).json()}
+        # remove all mergebot tags from the PR, then add just the ones which should be set
+        tags_after = (tags_before - mergebot_tags) | to_
+        # replace labels entirely
+        self('PUT', labels_endpoint, json={'labels': list(tags_after)})
 
-        if to_add:
-            self('POST', 'issues/{}/labels'.format(pr), json=list(to_add))
-
-        _logger.debug('change_tags(%s, %s, remove=%s, add=%s)', self._repo, pr, to_remove, to_add)
+        _logger.debug('change_tags(%s, %s, from=%s, to=%s)', self._repo, pr, tags_before, tags_after)
 
     def fast_forward(self, branch, sha):
         try:
diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index a6358234..709fb9a2 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -98,7 +98,6 @@ class Project(models.Model):
         to_remove = []
         for repo_id, pr, ids, from_, to_ in self.env.cr.fetchall():
             repo = Repos.browse(repo_id)
-            from_tags = _TAGS[from_ or False]
             to_tags = _TAGS[to_ or False]
 
             gh = ghs.get(repo)
@@ -106,11 +105,11 @@ class Project(models.Model):
                 gh = ghs[repo] = repo.github()
 
             try:
-                gh.change_tags(pr, from_tags, to_tags)
+                gh.change_tags(pr, to_tags)
             except Exception:
                 _logger.exception(
                     "Error while trying to change the tags of %s:%s from %s to %s",
-                    repo.name, pr, from_tags, to_tags,
+                    repo.name, pr, _TAGS[from_ or False], to_tags,
                 )
             else:
                 to_remove.extend(ids)
diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py
index 44153c27..b31c5660 100644
--- a/runbot_merge/tests/fake_github/__init__.py
+++ b/runbot_merge/tests/fake_github/__init__.py
@@ -502,29 +502,24 @@ class Repo(object):
             body=body, preload_content=False,
         )
 
-    def _add_labels(self, r, number):
+    def _get_labels(self, r, number):
         try:
             pr = self.issues[int(number)]
         except KeyError:
             return (404, None)
 
-        pr.labels.update(json.loads(r.body))
+        return (200, [{'name': label} for label in pr.labels])
+
+    def _reset_labels(self, r, number):
+        try:
+            pr = self.issues[int(number)]
+        except KeyError:
+            return (404, None)
+
+        pr.labels = set(json.loads(r.body)['labels'])
 
         return (200, {})
 
-    def _remove_label(self, _, number, label):
-        try:
-            pr = self.issues[int(number)]
-        except KeyError:
-            return (404, None)
-
-        try:
-            pr.labels.remove(werkzeug.urls.url_unquote(label))
-        except KeyError:
-            return (404, None)
-        else:
-            return (200, {})
-
     def _do_merge(self, r):
         body = json.loads(r.body) # {base, head, commit_message}
         if not body.get('commit_message'):
@@ -585,8 +580,8 @@ class Repo(object):
         ('GET', r'pulls/(?P<number>\d+)/reviews', _read_pr_reviews),
         ('GET', r'pulls/(?P<number>\d+)/commits', _read_pr_commits),
 
-        ('POST', r'issues/(?P<number>\d+)/labels', _add_labels),
-        ('DELETE', r'issues/(?P<number>\d+)/labels/(?P<label>.+)', _remove_label),
+        ('GET', r'issues/(?P<number>\d+)/labels', _get_labels),
+        ('PUT', r'issues/(?P<number>\d+)/labels', _reset_labels),
     ]
 
 class Issue(object):
diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py
index 795b64c5..dd4520c2 100644
--- a/runbot_merge/tests/remote.py
+++ b/runbot_merge/tests/remote.py
@@ -635,8 +635,58 @@ ct = itertools.count()
 
 Commit = collections.namedtuple('Commit', 'id tree message author committer parents')
 
+from odoo.tools.func import lazy_property
+class LabelsProxy(collections.abc.MutableSet):
+    def __init__(self, pr):
+        self._pr = pr
+
+    @property
+    def _labels(self):
+        pr = self._pr
+        r = pr._session.get('https://api.github.com/repos/{}/issues/{}/labels'.format(pr.repo.name, pr.number))
+        assert r.ok, r.json()
+        return {label['name'] for label in r.json()}
+
+    def __repr__(self):
+        return '<LabelsProxy %r>' % self._labels
+
+    def __eq__(self, other):
+        if isinstance(other, collections.abc.Set):
+            return other == self._labels
+        return NotImplemented
+
+    def __contains__(self, label):
+        return label in self._labels
+
+    def __iter__(self):
+        return iter(self._labels)
+
+    def __len__(self):
+        return len(self._labels)
+
+    def add(self, label):
+        pr = self._pr
+        r = pr._session.post('https://api.github.com/repos/{}/issues/{}/labels'.format(pr.repo.name, pr.number), json={
+            'labels': [label]
+        })
+        assert r.ok, r.json()
+
+    def discard(self, label):
+        pr = self._pr
+        r = pr._session.delete('https://api.github.com/repos/{}/issues/{}/labels/{}'.format(pr.repo.name, pr.number, label))
+        # discard should do nothing if the item didn't exist in the set
+        assert r.ok or r.status_code == 404, r.json()
+
+    def update(self, *others):
+        pr = self._pr
+        # because of course that one is not provided by MutableMapping...
+        r = pr._session.post('https://api.github.com/repos/{}/issues/{}/labels'.format(pr.repo.name, pr.number), json={
+            'labels': list(set(itertools.chain.from_iterable(others)))
+        })
+        assert r.ok, r.json()
+
 class PR:
-    __slots__ = ['number', '_branch', 'repo']
+    __slots__ = ['number', '_branch', 'repo', 'labels']
     def __init__(self, repo, branch, number):
         """
         :type repo: Repo
@@ -646,6 +696,7 @@ class PR:
         self.number = number
         self._branch = branch
         self.repo = repo
+        self.labels = LabelsProxy(self)
 
     @property
     def _session(self):
@@ -669,12 +720,6 @@ class PR:
     def state(self):
         return self._pr['state']
 
-    @property
-    def labels(self):
-        r = self._session.get('https://api.github.com/repos/{}/issues/{}/labels'.format(self.repo.name, self.number))
-        assert 200 <= r.status_code < 300, r.json()
-        return {label['name'] for label in r.json()}
-
     @property
     def comments(self):
         r = self._session.get('https://api.github.com/repos/{}/issues/{}/comments'.format(self.repo.name, self.number))
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index fa5686d5..49fc7050 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -2658,3 +2658,57 @@ class TestEmailFormatting:
             'github_login': 'Osmose99',
         })
         assert p1.formatted_email == 'Shultz <Osmose99@users.noreply.github.com>'
+
+class TestLabelling:
+    def test_desync(self, env, repo):
+        m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
+        repo.make_ref('heads/master', m)
+
+        c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'})
+        pr = repo.make_pr('gibberish', 'blahblah', target='master', ctid=c, user='user')
+
+        [pr_id] = env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', repo.name),
+            ('number', '=', pr.number),
+        ])
+        repo.post_status(c, 'success', 'legal/cla')
+        repo.post_status(c, 'success', 'ci/runbot')
+
+        run_crons(env)
+
+        assert pr.labels == {'seen 🙂', 'CI 🤖'}
+        # desync state and labels
+        pr.labels.remove('CI 🤖')
+
+        pr.post_comment('hansen r+', 'reviewer')
+        run_crons(env)
+
+        assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷'},\
+            "labels should be resynchronised"
+
+    def test_other_tags(self, env, repo):
+        m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
+        repo.make_ref('heads/master', m)
+
+        c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'})
+        pr = repo.make_pr('gibberish', 'blahblah', target='master', ctid=c, user='user')
+
+        # "foreign" labels
+        pr.labels.update(('L1', 'L2'))
+
+        [pr_id] = env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', repo.name),
+            ('number', '=', pr.number),
+        ])
+        repo.post_status(c, 'success', 'legal/cla')
+        repo.post_status(c, 'success', 'ci/runbot')
+
+        run_crons(env)
+
+        assert pr.labels == {'seen 🙂', 'CI 🤖', 'L1', 'L2'}, "should not lose foreign labels"
+
+        pr.post_comment('hansen r+', 'reviewer')
+        run_crons(env)
+
+        assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', 'L1', 'L2'},\
+            "should not lose foreign labels"