Repository: smarkets/marge-bot

Issue:
Re-approvals only applied after successful CI run
There is a comment related to this within `marge/job.py`, including for context:
```
# Re-approve the merge request, in case us pushing it has removed
# approvals. Note that there is a bit of a race; effectively
# approval can't be withdrawn after we've pushed (resetting
# approvals) and CI runs.
```

Occasionally CI may fail due to transient network issues that are unrelated to the change made. In this case, Marge will error out and not bother attempting to reapply any approvals. GitLab doesn't remove approvals on CI failure, so it doesn't quite make sense that this happens with Marge.

This also applies to any potential exception that might occur between the force push and applying approvals, we need to restart marge and then manually approve again.

I'm unaware as to whether there is a historical reason for why approvals are reapplied when they are, but could they no be applied immediately after the rebase?

Discussion:

Patch:
diff --git a/marge/app.py b/marge/app.py
index 576ade9..29b524e 100644
--- a/marge/app.py
+++ b/marge/app.py
@@ -178,7 +178,7 @@ def main(args=sys.argv[1:]):
                 add_reviewers=options.add_reviewers,
                 reapprove=options.impersonate_approvers,
                 embargo=options.embargo,
-                ci_timeout=timedelta(seconds=options.ci_timeout),
+                ci_timeout=options.ci_timeout,
             )
         )
 
diff --git a/marge/job.py b/marge/job.py
index b2d69fe..ae2b251 100644
--- a/marge/job.py
+++ b/marge/job.py
@@ -63,7 +63,7 @@ class MergeJob(object):
             log.exception('Unexpected Git error')
             merge_request.comment('Something seems broken on my local git repo; check my logs!')
             raise
-        except Exception:
+        except Exception as _ex:
             log.exception('Unexpected Exception')
             merge_request.comment("I'm broken on the inside, please somebody fix me... :cry:")
             self.unassign_from_mr(merge_request)
@@ -119,11 +119,6 @@ class MergeJob(object):
             log.info('Commit id to merge %r (into: %r)', actual_sha, target_sha)
             time.sleep(5)
 
-            if source_project.only_allow_merge_if_pipeline_succeeds:
-                self.wait_for_ci_to_pass(source_project.id, actual_sha)
-                log.info('CI passed!')
-                time.sleep(2)
-
             sha_now = Commit.last_on_branch(source_project.id, merge_request.source_branch, api).id
             # Make sure no-one managed to race and push to the branch in the
             # meantime, because we're about to impersonate the approvers, and
@@ -133,13 +128,18 @@ class MergeJob(object):
             # Re-approve the merge request, in case us pushing it has removed
             # approvals. Note that there is a bit of a race; effectively
             # approval can't be withdrawn after we've pushed (resetting
-            # approvals) and CI runs.
+            # approvals)
             if self.opts.reapprove:
                 # approving is not idempotent, so we need to check first that there are no approvals,
                 # otherwise we'll get a failure on trying to re-instate the previous approvals
                 current_approvals = merge_request.fetch_approvals()
                 if not current_approvals.sufficient:
                     approvals.reapprove()
+
+            if source_project.only_allow_merge_if_pipeline_succeeds:
+                self.wait_for_ci_to_pass(source_project.id, actual_sha)
+                log.info('CI passed!')
+                time.sleep(2)
             try:
                 merge_request.accept(remove_branch=True, sha=actual_sha)
             except gitlab.NotAcceptable as err:


Test patch:
diff --git a/tests/test_app.py b/tests/test_app.py
index d8a4705..ed8e64b 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -1,10 +1,141 @@
+import contextlib
 import datetime
+import os
+import re
+import shlex
+import unittest.mock as mock
+from functools import wraps
 
-from marge.app import time_interval
+import pytest
+
+import marge.app as app
+import marge.bot as bot
+import marge.interval as interval
+import marge.job as job
+
+import tests.gitlab_api_mock as gitlab_mock
+from tests.test_user import INFO as user_info
+
+
+@contextlib.contextmanager
+def env(**kwargs):
+    original = os.environ.copy()
+
+    os.environ.clear()
+    for k, v in kwargs.items():
+        os.environ[k] = v
+
+    yield
+
+    os.environ.clear()
+    for k, v in original.items():
+        os.environ[k] = v
+
+
+@contextlib.contextmanager
+def main(cmdline=''):
+    def api_mock(gitlab_url, auth_token):
+        assert gitlab_url == 'http://foo.com'
+        assert auth_token in ('NON-ADMIN-TOKEN', 'ADMIN-TOKEN')
+        api = gitlab_mock.Api(gitlab_url=gitlab_url, auth_token=auth_token, initial_state='initial')
+        user_info_for_token = dict(user_info, is_admin=auth_token == 'ADMIN-TOKEN')
+        api.add_user(user_info_for_token, is_current=True)
+        return api
+
+    class DoNothingBot(bot.Bot):
+        instance = None
+
+        def start(self):
+            assert self.__class__.instance is None
+            self.__class__.instance = self
+
+        @property
+        def config(self):
+            return self._config
+
+    with mock.patch('marge.bot.Bot', new=DoNothingBot), mock.patch('marge.gitlab.Api', new=api_mock):
+        app.main(args=shlex.split(cmdline))
+        the_bot = DoNothingBot.instance
+        assert the_bot is not None
+        yield the_bot
+
+
+def test_default_values():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main() as bot:
+            assert bot.user.info == user_info
+            assert bot.config.project_regexp == re.compile('.*')
+            assert bot.config.git_timeout == datetime.timedelta(seconds=120)
+            assert bot.config.merge_opts == job.MergeJobOptions.default()
+
+def test_embargo():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--embargo="Fri 1pm-Mon 7am"') as bot:
+            assert bot.config.merge_opts == job.MergeJobOptions.default(
+               embargo=interval.IntervalUnion.from_human('Fri 1pm-Mon 7am'),
+            )
+
+def test_add_tested():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--add-tested') as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(add_tested=True)
+
+def test_add_part_of():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--add-part-of') as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(add_part_of=True)
+
+def test_add_reviewers():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with pytest.raises(AssertionError):
+            with main('--add-reviewers') as bot:
+                pass
+
+    with env(MARGE_AUTH_TOKEN="ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--add-reviewers') as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(add_reviewers=True)
+
+
+def test_impersonate_approvers():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with pytest.raises(AssertionError):
+            with main('--impersonate-approvers') as bot:
+                pass
+
+    with env(MARGE_AUTH_TOKEN="ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--impersonate-approvers') as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(reapprove=True)
+
+
+def test_project_regexp():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main("--project-regexp='foo.*bar'") as bot:
+            assert bot.config.project_regexp == re.compile('foo.*bar')
+
+def test_ci_timeout():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main("--ci-timeout 5m") as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(ci_timeout=datetime.timedelta(seconds=5*60))
+
+def test_deprecated_max_ci_time_in_minutes():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main("--max-ci-time-in-minutes=5") as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(ci_timeout=datetime.timedelta(seconds=5*60))
+
+def test_git_timeout():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main("--git-timeout '150 s'") as bot:
+            assert bot.config.git_timeout == datetime.timedelta(seconds=150)
 
 
 # FIXME: I'd reallly prefer this to be a doctest, but adding --doctest-modules
 # seems to seriously mess up the test run
 def test_time_interval():
     _900s = datetime.timedelta(0, 900)
-    assert [time_interval(x) for x in ['15min', '15min', '.25h', '900s']] == [_900s] * 4
+    assert [app.time_interval(x) for x in ['15min', '15m', '.25h', '900s']] == [_900s] * 4
diff --git a/tests/test_job.py b/tests/test_job.py
index e9b0639..4e9031b 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -88,6 +88,11 @@ class MockLab(object):
            Ok(_commit(id=rewritten_sha, status='success')),
            from_state=['passed', 'merged'],
         )
+        api.add_transition(
+           GET('/projects/1234/repository/branches/useless_new_feature'),
+           Ok({'commit': _commit(id=rewritten_sha, status='running')}),
+           from_state='pushed',
+        )
         api.add_transition(
            GET('/projects/1234/repository/branches/useless_new_feature'),
            Ok({'commit': _commit(id=rewritten_sha, status='success')}),
@@ -192,14 +197,14 @@ class TestRebaseAndAccept(object):
         api.add_transition(
            GET('/projects/1234/repository/branches/useless_new_feature'),
            Ok({'commit': _commit(id=new_branch_head_sha, status='success')}),
-           from_state='passed', to_state='passed_but_head_changed'
+           from_state='pushed', to_state='pushed_but_head_changed'
         )
         with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
             with mocklab.expected_failure("Someone pushed to branch while we were trying to merge"):
                 job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False))
                 job.execute()
 
-        assert api.state == 'passed_but_head_changed'
+        assert api.state == 'pushed_but_head_changed'
         assert api.notes == ["I couldn't merge this branch: Someone pushed to branch while we were trying to merge"]
 
     def test_succeeds_second_time_if_master_moved(self, time_sleep):