diff --git a/marge/approvals.py b/marge/approvals.py index 218de7f9..798684b7 100644 --- a/marge/approvals.py +++ b/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 not owner_globs: + 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 not codeowners: + 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,6 +117,14 @@ 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. @@ -114,11 +132,14 @@ def reapprove(self): (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) diff --git a/marge/merge_request.py b/marge/merge_request.py index bce1bbf8..d678ea41 100644 --- a/marge/merge_request.py +++ b/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 approvals.codeowners: + 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): diff --git a/setup.py b/setup.py index b5a9d800..0cbac74d 100644 --- a/setup.py +++ b/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, diff --git a/tests/test_approvals.py b/tests/test_approvals.py index ce52ebf2..6185a64f 100644 --- a/tests/test_approvals.py +++ b/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))