From 355d1dcd8b81d811b73e578aafbb4ceffb479f54 Mon Sep 17 00:00:00 2001 From: Lasse Mammen Date: Mon, 27 Apr 2020 08:32:25 +0100 Subject: [PATCH] Minimum approval config --- README.md | 19 ++++++++++++------- marge/approvals.py | 31 ++++++++++++++++++++++--------- tests/test_approvals.py | 38 ++++++++++++++++++++------------------ 3 files changed, 54 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index c975e04a..9ecc4a88 100644 --- a/README.md +++ b/README.md @@ -193,19 +193,24 @@ ssh-keygen -t ed25519 -C marge-bot@invalid -f marge-bot-ssh-key -P '' Add the public key (`marge-bot-ssh-key.pub`) to the user's `SSH Keys` in GitLab and keep the private one handy. -### Per project configuration +### Merge Approvals on Gitlab CE On GitLab enterprise the [merge request approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html) provide the information how many approvals from whom are needed for -a merge request. On GitlLab CE this is done via a configuration file -called `.marge-bot.yml`. Currently Marge uses the config file from master -as config for all merge request. +a merge request. -The `.marge-bot.yml` config currently only supports `approver_count`: -```yaml +On GitlLab CE this is done via the merge request's award API (thumbup action) and the +[CODEOWNERS](https://docs.gitlab.com/ee/user/project/code_owners.html) file. +Appropriate approvers will be determined from that file and the changes in the MR and minimum approvers can be set +by adding a commented line anywhere in that file that contains: -approver_count: 3 # number of "thumbs up" needed, defaults to 1 ``` +# MARGEBOT_MINIMUM_APPROVERS = 2 +``` + +Adjust for your needs. If no CODEOWNERS file exist or no matched owners the approval flow will be disabled. If no minimum +approvers is set, all matched users from CODEOWNERS will be required to approve. + ### Running marge-bot in docker (what we do) diff --git a/marge/approvals.py b/marge/approvals.py index 798684b7..48bc1302 100644 --- a/marge/approvals.py +++ b/marge/approvals.py @@ -1,5 +1,6 @@ import logging as log import fnmatch +import re import shlex from . import gitlab @@ -26,24 +27,29 @@ 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: + owner_file = self.get_codeowners_ce() + if not owner_file['owners']: 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()) + code_owners = self.determine_responsible_owners(owner_file['owners'], self.get_changes_ce()) - if not codeowners: + if not code_owners: log.info("No matched code owners, continuing without approvers flow") return dict(self._info, approvals_left=0, approved_by=[], codeowners=[]) 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) + up_votes = [e for e in awards if e['name'] == 'thumbsup' and e['user']['username'] in code_owners] - return dict(self._info, approvals_left=approvals_left, approved_by=up_votes, codeowners=codeowners) + approvals_required = len(code_owners) + + if owner_file['approvals_required'] > 0: + approvals_required = owner_file['approvals_required'] + + approvals_left = max(approvals_required - len(up_votes), 0) + + return dict(self._info, approvals_left=approvals_left, approved_by=up_votes, codeowners=code_owners) def determine_responsible_owners(self, owners_glob, changes): owners = set([]) @@ -77,11 +83,18 @@ def get_awards_ce(self): def get_codeowners_ce(self): config_file = self._api.repo_file_get(self.project_id, "CODEOWNERS", "master") owner_globs = {} + required = 0 + required_regex = re.compile('.*MARGEBOT_MINIMUM_APPROVERS *= *([0-9]+)') if config_file is None: return owner_globs for line in config_file['content'].splitlines(): + if 'MARGEBOT_' in line: + match = required_regex.match(line) + if match: + required = int(match.group(1)) + if line != "" and not line.startswith(' ') and not line.startswith('#'): elements = shlex.split(line) glob = elements.pop(0) @@ -90,7 +103,7 @@ def get_codeowners_ce(self): for user in elements: owner_globs[glob].add(user.strip('@')) - return owner_globs + return {"approvals_required": required, "owners": owner_globs} @property def iid(self): diff --git a/tests/test_approvals.py b/tests/test_approvals.py index 6185a64f..36d064dd 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -11,6 +11,7 @@ CODEOWNERS = { "content": """ +# MARGEBOT_MINIMUM_APPROVERS = 2 # This is an example code owners file, lines starting with a `#` will # be ignored. @@ -24,6 +25,7 @@ # pylint: disable=anomalous-backslash-in-string CODEOWNERS_FULL = { "content": """ +# MARGEBOT_MINIMUM_APPROVERS=3 # This is an example code owners file, lines starting with a `#` will # be ignored. @@ -49,10 +51,6 @@ # owner for the LICENSE file LICENSE @legal this_does_not_match janedoe@gitlab.com -# Group names can be used to match groups and nested groups to specify -# them as owners for a file -README @group @group/with-nested/subgroup - # Ending a path in a `/` will specify the code owners for every file # nested in that directory, on any level /docs/ @all-docs @@ -222,7 +220,7 @@ def test_fetch_from_merge_request_ce_compat(self): 'id': 74, 'iid': 6, 'project_id': 1234, - 'approvals_left': 2, + 'approvals_left': 1, 'approved_by': [AWARDS[1]], 'codeowners': {'default-codeowner', 'ebert', 'test-user1'}, } @@ -280,17 +278,20 @@ def test_approvals_ce_get_codeowners_full(self): approvals = Approvals(api, {'id': 74, 'iid': 6, 'project_id': 1234}) assert approvals.get_codeowners_ce() == { - '#file_with_pound.rb': {'owner-file-with-pound'}, - '*': {'default-codeowner'}, - '*.rb': {'ruby-owner'}, - '/config/': {'config-owner'}, - '/docs/': {'all-docs'}, - '/docs/*': {'root-docs'}, - 'CODEOWNERS': {'owners', 'multiple', 'code'}, - 'LICENSE': {'this_does_not_match', 'janedoe@gitlab.com', 'legal'}, - 'README': {'group', 'group/with-nested/subgroup'}, - 'lib/': {'lib-owner'}, - 'path with spaces/': {'space-owner'} + 'approvals_required': 3, + 'owners': { + '#file_with_pound.rb': {'owner-file-with-pound'}, + '*': {'default-codeowner'}, + '*.rb': {'ruby-owner'}, + '/config/': {'config-owner'}, + '/docs/': {'all-docs'}, + '/docs/*': {'root-docs'}, + 'CODEOWNERS': {'owners', 'multiple', 'code'}, + 'LICENSE': {'this_does_not_match', 'janedoe@gitlab.com', 'legal'}, + 'README': {'group', 'group/with-nested/subgroup'}, + 'lib/': {'lib-owner'}, + 'path with spaces/': {'space-owner'} + } } def test_approvals_ce_get_codeowners_wildcard(self): @@ -301,7 +302,8 @@ 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'} + 'approvals_required': 2, + 'owners': {'*': {'default-codeowner', 'test-user1', 'ebert'}, 'unmatched/*': {'test5'}} } @patch('marge.approvals.Approvals.get_awards_ce', Mock(return_value=AWARDS)) @@ -316,5 +318,5 @@ def test_approvals_ce(self): result = approvals.get_approvers_ce() - assert result['approvals_left'] == 2 + assert result['approvals_left'] == 1 assert len(result['approved_by']) == 1