More error handling and comment on MR with possible reviewers

Author: lkm

marge/approvals.py

@@ -1,7 +1,6 @@
-import yaml
 import logging as log
-import shlex
 import fnmatch
+import shlex
 
 from . import gitlab
 
@@ -27,14 +26,24 @@ def refetch_info(self):
     def get_approvers_ce(self):
         """get approvers status using thumbs on merge request
         """
+        owner_globs = self.get_codeowners_ce()
+        if len(owner_globs) == 0:
+            log.info("No CODEOWNERS file in master, continuing without approvers flow")
+            return dict(self._info, approvals_left=0, approved_by=[], codeowners=[])
+
+        codeowners = self.determine_responsible_owners(owner_globs, self.get_changes_ce())
+
+        if len(codeowners) == 0:
+            log.info("No matched code owners, continuing without approvers flow")
+            return dict(self._info, approvals_left=0, approved_by=[], codeowners=[])
 
-        codeowners = self.determine_responsible_owners(self.get_codeowners_ce(), self.get_changes_ce())
         awards = self.get_awards_ce()
 
         up_votes = [e for e in awards if e['name'] == 'thumbsup' and e['user']['username'] in codeowners]
         approver_count = len(codeowners)
         approvals_left = max(approver_count - len(up_votes), 0)
-        return dict(self._info, approvals_left=approvals_left, approved_by=up_votes)
+
+        return dict(self._info, approvals_left=approvals_left, approved_by=up_votes, codeowners=codeowners)
 
     def determine_responsible_owners(self, owners_glob, changes):
         owners = set([])
@@ -44,6 +53,7 @@ def determine_responsible_owners(self, owners_glob, changes):
             owners.update(owners_glob['*'])
 
         if 'changes' not in changes:
+            log.info("No changes in merge request!?")
             return owners
 
         for change in changes['changes']:
@@ -71,9 +81,9 @@ def get_codeowners_ce(self):
         if config_file is None:
             return owner_globs
 
-        for line in config_file.splitlines():
+        for line in config_file['content'].splitlines():
             if line != "" and not line.startswith(' ') and not line.startswith('#'):
-                elements = shlex.split(line) # line.split()
+                elements = shlex.split(line)
                 glob = elements.pop(0)
                 owner_globs.setdefault(glob, set([]))
 
@@ -107,18 +117,29 @@ def approver_ids(self):
         """Return the uids of the approvers."""
         return [who['user']['id'] for who in self.info['approved_by']]
 
+    @property
+    def codeowners(self):
+        """Only used for gitlab CE"""
+        if 'approvers' in self.info:
+            return self.info['codeowners']
+
+        return []
+
     def reapprove(self):
         """Impersonates the approvers and re-approves the merge_request as them.
 
         The idea is that we want to get the approvers, push the rebased branch
         (which may invalidate approvals, depending on GitLab settings) and then
         restore the approval status.
         """
-        if self._api.version().release >= (9, 2, 2):
+        gitlab_version = self._api.version()
+
+        if gitlab_version.release >= (9, 2, 2):
             approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(self)
         else:
             # GitLab botched the v4 api before 9.2.3
             approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(self)
 
-        for uid in self.approver_ids:
-            self._api.call(POST(approve_url), sudo=uid)
+        if gitlab_version.is_ee:
+            for uid in self.approver_ids:
+                self._api.call(POST(approve_url), sudo=uid)

marge/merge_request.py

@@ -188,6 +188,19 @@ def fetch_approvals(self):
         info = {'id': self.id, 'iid': self.iid, 'project_id': self.project_id}
         approvals = Approvals(self.api, info)
         approvals.refetch_info()
+
+        if not self._api.version().is_ee and approvals.approvals_left > 0 and len(approvals.codeowners) > 0:
+            reviewer_string = ''
+            if len(approvals.codeowners) == 1:
+                reviewer_string = '@' + approvals.codeowners[0]
+            else:
+                reviewer_ats = ["@" + reviewer for reviewer in approvals.codeowners]
+                reviewer_string = '{} or {}'.format(', '.join(reviewer_ats[:-1]), reviewer_ats[-1])
+
+            self.comment(
+                "I can't merge without approval. Please ask {0} to review".format(reviewer_string)
+            )
+
         return approvals
 
     def fetch_commits(self):

setup.py

@@ -1,6 +1,6 @@
 import os
 from distutils.core import setup
-VERSION = open(os.path.join(os.path.dirname(__file__),  'version')).read().strip()
+VERSION = open(os.path.join(os.path.dirname(__file__), 'version')).read().strip()
 setup(
     name='marge',
     version=VERSION,

tests/test_approvals.py

@@ -9,7 +9,8 @@
 # testing this here is more convenient
 from marge.job import CannotMerge, _get_reviewer_names_and_emails
 
-CODEOWNERS = """
+CODEOWNERS = {
+    "content": """
 # This is an example code owners file, lines starting with a `#` will
 # be ignored.
 
@@ -18,8 +19,11 @@
 
 unmatched/* @test5
 """
+}
 
-CODEOWNERS_FULL = """
+# pylint: disable=anomalous-backslash-in-string
+CODEOWNERS_FULL = {
+    "content": """
 # This is an example code owners file, lines starting with a `#` will
 # be ignored.
 
@@ -68,7 +72,8 @@
 
 # If the path contains spaces, these need to be escaped like this:
 path\ with\ spaces/ @space-owner
-"""
+"""  # noqa: W605
+}
 
 AWARDS = [
     {
@@ -214,7 +219,12 @@ def test_fetch_from_merge_request_ce_compat(self):
 
         api.call.assert_not_called()
         assert approvals.info == {
-            'id': 74, 'iid': 6, 'project_id': 1234, 'approvals_left': 2, 'approved_by': [AWARDS[1]],
+            'id': 74,
+            'iid': 6,
+            'project_id': 1234,
+            'approvals_left': 2,
+            'approved_by': [AWARDS[1]],
+            'codeowners': {'default-codeowner', 'ebert', 'test-user1'},
         }
 
     def test_properties(self):
@@ -290,7 +300,9 @@ def test_approvals_ce_get_codeowners_wildcard(self):
 
         approvals = Approvals(api, {'id': 74, 'iid': 6, 'project_id': 1234})
 
-        assert approvals.get_codeowners_ce() == {'*': set(['default-codeowner', 'test-user1', 'ebert']), 'unmatched/*': {'test5'}}
+        assert approvals.get_codeowners_ce() == {
+            '*': set(['default-codeowner', 'test-user1', 'ebert']), 'unmatched/*': {'test5'}
+        }
 
     @patch('marge.approvals.Approvals.get_awards_ce', Mock(return_value=AWARDS))
     @patch('marge.approvals.Approvals.get_changes_ce', Mock(return_value=CHANGES))