From e7d1f1523d131e49292cfdf1ed506e2567e54887 Mon Sep 17 00:00:00 2001 From: Lasse Mammen Date: Fri, 24 Apr 2020 15:05:53 +0100 Subject: [PATCH] Use CODEOWNERS and changes to determine who can approve At the moment all matches from codeowners has to be available to approve --- marge/approvals.py | 63 ++++++++++++--- tests/test_approvals.py | 168 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 218 insertions(+), 13 deletions(-) diff --git a/marge/approvals.py b/marge/approvals.py index 3bd73fba..218de7f9 100644 --- a/marge/approvals.py +++ b/marge/approvals.py @@ -1,5 +1,7 @@ import yaml import logging as log +import shlex +import fnmatch from . import gitlab @@ -20,28 +22,65 @@ def refetch_info(self): if gitlab_version.is_ee: self._info = self._api.call(GET(approver_url)) else: - self.get_approvers_ce() + self._info = self.get_approvers_ce() def get_approvers_ce(self): """get approvers status using thumbs on merge request """ - config_file = self._api.repo_file_get(self.project_id, ".marge-bot.yml", "master") - if config_file is None: - log.info('Project id %s missing .marge-bot.yaml', self.project_id) - config = {} - else: - config = yaml.load(config_file["content"]) + 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) + + def determine_responsible_owners(self, owners_glob, changes): + owners = set([]) + + # Always add global users + if '*' in owners_glob: + owners.update(owners_glob['*']) + + if 'changes' not in changes: + return owners + + for change in changes['changes']: + for glob, users in owners_glob.items(): + if 'new_path' in change and fnmatch.fnmatch(change['new_path'], glob): + owners.update(users) + return owners + def get_changes_ce(self): + changes_url = '/projects/{0.project_id}/merge_requests/{0.iid}/changes' + changes_url = changes_url.format(self) + + return self._api.call(GET(changes_url)) + + def get_awards_ce(self): emoji_url = '/projects/{0.project_id}/merge_requests/{0.iid}/award_emoji' emoji_url = emoji_url.format(self) - emoji = self._api.call(GET(emoji_url)) + return self._api.call(GET(emoji_url)) - up_votes = [e for e in emoji if e['name'] == 'thumbsup'] - approver_count = config.get('approver_count', 1) - approvals_left = max(approver_count - len(up_votes), 0) - self._info = dict(self._info, approvals_left=approvals_left, approved_by=up_votes) + def get_codeowners_ce(self): + config_file = self._api.repo_file_get(self.project_id, "CODEOWNERS", "master") + owner_globs = {} + + if config_file is None: + return owner_globs + + for line in config_file.splitlines(): + if line != "" and not line.startswith(' ') and not line.startswith('#'): + elements = shlex.split(line) # line.split() + glob = elements.pop(0) + owner_globs.setdefault(glob, set([])) + + for user in elements: + owner_globs[glob].add(user.strip('@')) + + return owner_globs @property def iid(self): diff --git a/tests/test_approvals.py b/tests/test_approvals.py index 2366bbe2..ce52ebf2 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -9,6 +9,124 @@ # testing this here is more convenient from marge.job import CannotMerge, _get_reviewer_names_and_emails +CODEOWNERS = """ +# This is an example code owners file, lines starting with a `#` will +# be ignored. + +* @default-codeowner @test-user1 +* @test-user1 @ebert + +unmatched/* @test5 +""" + +CODEOWNERS_FULL = """ +# This is an example code owners file, lines starting with a `#` will +# be ignored. + +# app/ @commented-rule + +# We can specify a default match using wildcards: +* @default-codeowner + +# Rules defined later in the file take precedence over the rules +# defined before. +# This will match all files for which the file name ends in `.rb` +*.rb @ruby-owner + +# Files with a `#` can still be accesssed by escaping the pound sign +\#file_with_pound.rb @owner-file-with-pound + +# Multiple codeowners can be specified, separated by spaces or tabs +CODEOWNERS @multiple @code @owners + +# Both usernames or email addresses can be used to match +# users. Everything else will be ignored. For example this will +# specify `@legal` and a user with email `janedoe@gitlab.com` as the +# 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 + +# Ending a path in `/*` will specify code owners for every file in +# that directory, but not nested deeper. This will match +# `docs/index.md` but not `docs/projects/index.md` +/docs/* @root-docs + +# This will make a `lib` directory nested anywhere in the repository +# match +lib/ @lib-owner + +# This will only match a `config` directory in the root of the +# repository +/config/ @config-owner + +# If the path contains spaces, these need to be escaped like this: +path\ with\ spaces/ @space-owner +""" + +AWARDS = [ + { + "id": 1, + "name": "thumbsdown", + "user": { + "name": "Test User 1", + "username": "test-user1", + "id": 3, + "state": "active", + }, + "created_at": "2020-04-24T13:16:23.614Z", + "updated_at": "2020-04-24T13:16:23.614Z", + "awardable_id": 11, + "awardable_type": "MergeRequest" + }, + { + "id": 2, + "name": "thumbsup", + "user": { + "name": "Roger Ebert", + "username": "ebert", + "id": 2, + "state": "active", + }, + "created_at": "2020-04-24T13:16:23.614Z", + "updated_at": "2020-04-24T13:16:23.614Z", + "awardable_id": 12, + "awardable_type": "MergeRequest" + } +] + +CHANGES = { + "changes": [ + { + "old_path": "README", + "new_path": "README", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "diff": "", + }, + { + "old_path": "main.go", + "new_path": "main.go", + "a_mode": "100644", + "b_mode": "100644", + "new_file": False, + "renamed_file": False, + "deleted_file": False, + "diff": "" + } + ] +} + + INFO = { "id": 5, "iid": 6, @@ -83,17 +201,20 @@ def test_fetch_from_merge_request(self): )) assert approvals.info == INFO + @patch('marge.approvals.Approvals.get_awards_ce', Mock(return_value=AWARDS)) + @patch('marge.approvals.Approvals.get_changes_ce', Mock(return_value=CHANGES)) def test_fetch_from_merge_request_ce_compat(self): api = self.api api.version = Mock(return_value=Version.parse('9.2.3')) api.call = Mock() + api.repo_file_get = Mock(return_value=CODEOWNERS) merge_request = MergeRequest(api, {'id': 74, 'iid': 6, 'project_id': 1234}) approvals = merge_request.fetch_approvals() api.call.assert_not_called() assert approvals.info == { - 'id': 74, 'iid': 6, 'project_id': 1234, 'approvals_left': 0, 'approved_by': [], + 'id': 74, 'iid': 6, 'project_id': 1234, 'approvals_left': 2, 'approved_by': [AWARDS[1]], } def test_properties(self): @@ -140,3 +261,48 @@ def test_approvals_succeeds_with_independent_author(self, user_fetch_by_id): 'Administrator ', 'Roger Ebert ', ] + + def test_approvals_ce_get_codeowners_full(self): + api = self.api + api.version = Mock(return_value=Version.parse('9.2.3')) + api.repo_file_get = Mock(return_value=CODEOWNERS_FULL) + + 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'} + } + + def test_approvals_ce_get_codeowners_wildcard(self): + api = self.api + api.version = Mock(return_value=Version.parse('9.2.3')) + api.repo_file_get = Mock(return_value=CODEOWNERS) + + approvals = Approvals(api, {'id': 74, 'iid': 6, 'project_id': 1234}) + + 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)) + def test_approvals_ce(self): + api = self.api + api.version = Mock(return_value=Version.parse('9.2.3')) + api.repo_file_get = Mock(return_value=CODEOWNERS) + + merge_request = MergeRequest(api, {'id': 74, 'iid': 6, 'project_id': 1234}) + approvals = merge_request.fetch_approvals() + + result = approvals.get_approvers_ce() + + assert result['approvals_left'] == 2 + assert len(result['approved_by']) == 1