Repository: smarkets/marge-bot

Issue:
Handle GitLab setting merge request as WIP after pushing
Because of [this GitLab feature](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8124), we can have the following interaction:

  1. Someone creates a merge request containing a commit with WIP in its name. GitLab doesn't consider the Merge Request as WIP at this point.
  1. Merge request is assigned to Marge; it checks that the merge requests is not WIP and proceeds.
  1 The branch is rebased and pushed again.
  1. Because we have just pushed a commit that contains WIP (even if it was in one of the original commits), GitLab marks the merge request as WIP (it even reports "Marge Bot marked as Work In Progress").
  1. After CI passes, she tries to merge, but GitLab now refuses to merge and we fail with "had some issue with gitlab"

We should test for WIP status before merging or, perhaps better, after the merge failed and we don't know why.

Discussion:

Patch:
diff --git a/marge/job.py b/marge/job.py
index 8a562d8..b2078d8 100644
--- a/marge/job.py
+++ b/marge/job.py
@@ -153,6 +153,38 @@ class MergeJob(object):
             except gitlab.Unauthorized:
                 log.warning('Unauthorized!')
                 raise CannotMerge('My user cannot accept merge requests!')
+            except gitlab.NotFound as e:
+                log.warning('Not Found!: %s', e)
+                merge_request.refetch_info()
+                if merge_request.state == 'merged':
+                    # someone must have hit "merge when build succeeds" and we lost the race,
+                    # the branch is gone and we got a 404. Anyway, our job here is done.
+                    # (see #33)
+                    rebased_into_up_to_date_target_branch = True
+                else:
+                    log.warning('For the record, merge request state is %r', merge_request.state)
+                    raise
+            except gitlab.MethodNotAllowed as e:
+                log.warning('Not Allowed!: %s', e)
+                merge_request.refetch_info()
+                if merge_request.work_in_progress:
+                    raise CannotMerge(
+                        'The request was marked as WIP as I was processing it (maybe a WIP commit?)'
+                    )
+                elif merge_request.state == 'reopened':
+                    raise CannotMerge(
+                        'GitLab refused to merge this branch. I suspect that a Push Rule or a git-hook '
+                        'is rejecting my commits; maybe my email needs to be white-listed?'
+                    )
+                elif merge_request.state == 'closed':
+                    raise CannotMerge('Someone closed the merge request while I was attempting to merge it.')
+                elif merge_request.state == 'merged':
+                    # We are not covering any observed behaviour here, but if at this
+                    # point the request is merged, our job is done, so no need to complain
+                    log.info('Merge request is already merged, someone was faster!')
+                    rebased_into_up_to_date_target_branch = True
+                else:
+                    raise CannotMerge("Gitlab refused to merge this request and I don't know why!")
             except gitlab.ApiError:
                 log.exception('Unanticipated ApiError from Gitlab on merge attempt')
                 raise CannotMerge('had some issue with gitlab, check my logs...')


Test patch:
diff --git a/tests/test_job.py b/tests/test_job.py
index 8e6bf77..984c9c4 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -246,6 +246,119 @@ class TestRebaseAndAccept(object):
         assert api.state == 'merged'
         assert api.notes == ["My job would be easier if people didn't jump the queue and pushed directly... *sigh*"]
 
+    def test_handles_races_for_merging(self, time_sleep):
+        api, mocklab = self.api, self.mocklab
+        rewritten_sha = mocklab.rewritten_sha
+        api.add_transition(
+            PUT(
+                '/projects/1234/merge_requests/54/merge',
+                dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True),
+            ),
+            Error(marge.gitlab.NotFound(404, {'message': '404 Branch Not Found'})),
+            from_state='passed', to_state='someone_else_merged',
+        )
+        api.add_merge_request(
+            dict(mocklab.merge_request_info, state='merged'),
+            from_state='someone_else_merged',
+        )
+        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+            job = self.make_job()
+            job.execute()
+        assert api.state == 'someone_else_merged'
+        assert api.notes == []
+
+    def test_handles_request_becoming_wip_after_push(self, time_sleep):
+        api, mocklab = self.api, self.mocklab
+        rewritten_sha = mocklab.rewritten_sha
+        api.add_transition(
+            PUT(
+                '/projects/1234/merge_requests/54/merge',
+                dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True),
+            ),
+            Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})),
+            from_state='passed', to_state='now_is_wip',
+        )
+        api.add_merge_request(
+            dict(mocklab.merge_request_info, work_in_progress=True),
+            from_state='now_is_wip',
+        )
+        message = 'The request was marked as WIP as I was processing it (maybe a WIP commit?)'
+        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+            with mocklab.expected_failure(message):
+                job = self.make_job()
+                job.execute()
+        assert api.state == 'now_is_wip'
+        assert api.notes == ["I couldn't merge this branch: %s" % message]
+
+    def test_guesses_git_hook_error_on_merge_refusal(self, time_sleep):
+        api, mocklab = self.api, self.mocklab
+        rewritten_sha = mocklab.rewritten_sha
+        api.add_transition(
+            PUT(
+                '/projects/1234/merge_requests/54/merge',
+                dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True),
+            ),
+            Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})),
+            from_state='passed', to_state='rejected_by_git_hook',
+        )
+        api.add_merge_request(
+            dict(mocklab.merge_request_info, state='reopened'),
+            from_state='rejected_by_git_hook',
+        )
+        message = (
+            'GitLab refused to merge this branch. I suspect that a Push Rule or a git-hook '
+            'is rejecting my commits; maybe my email needs to be white-listed?'
+        )
+        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+            with mocklab.expected_failure(message):
+                job = self.make_job()
+                job.execute()
+        assert api.state == 'rejected_by_git_hook'
+        assert api.notes == ["I couldn't merge this branch: %s" % message]
+
+    def test_guesses_git_hook_error_on_merge_refusal(self, time_sleep):
+        api, mocklab = self.api, self.mocklab
+        rewritten_sha = mocklab.rewritten_sha
+        api.add_transition(
+            PUT(
+                '/projects/1234/merge_requests/54/merge',
+                dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True),
+            ),
+            Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})),
+            from_state='passed', to_state='oops_someone_closed_it',
+        )
+        api.add_merge_request(
+            dict(mocklab.merge_request_info, state='closed'),
+            from_state='oops_someone_closed_it',
+        )
+        message = 'Someone closed the merge request while I was attempting to merge it.'
+        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+            with mocklab.expected_failure(message):
+                job = self.make_job()
+                job.execute()
+        assert api.state == 'oops_someone_closed_it'
+        assert api.notes == ["I couldn't merge this branch: %s" % message]
+
+    def test_tells_explicitly_that_gitlab_refused_to_merge(self, time_sleep):
+        api, mocklab = self.api, self.mocklab
+        rewritten_sha = mocklab.rewritten_sha
+        api.add_transition(
+            PUT(
+                '/projects/1234/merge_requests/54/merge',
+                dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True),
+            ),
+            Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})),
+            from_state='passed', to_state='rejected_for_misterious_reasons',
+        )
+        message = "Gitlab refused to merge this request and I don't know why!"
+        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+            with mocklab.expected_failure(message):
+                job = self.make_job()
+                job.execute()
+        assert api.state == 'rejected_for_misterious_reasons'
+        assert api.notes == ["I couldn't merge this branch: %s" % message]
+
+
     def test_wont_merge_wip_stuff(self, time_sleep):
         api, mocklab = self.api, self.mocklab
         wip_merge_request = dict(mocklab.merge_request_info, work_in_progress=True)