From ea98130300be062c89766abadc96c2f903caa6ea Mon Sep 17 00:00:00 2001 From: Lasse Mammen Date: Thu, 18 Feb 2021 14:48:09 +0000 Subject: [PATCH] Use a combination of awards api and approvals (only >13.2) for approval --- README.md | 19 ++++ marge/approvals.py | 151 +++++++++++++++++++++++++++--- marge/gitlab.py | 39 +++++++- marge/job.py | 16 +++- tests/test_approvals.py | 197 +++++++++++++++++++++++++++++++++++++++- 5 files changed, 401 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 9f1b4043..99f1fa83 100644 --- a/README.md +++ b/README.md @@ -200,6 +200,25 @@ 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. +### 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 combination of the merge request's award API (thumbup action), approvals (in gitlab > 13.2.0) 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: + +``` +# 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) Assuming you have already got docker installed, the quickest and most minimal diff --git a/marge/approvals.py b/marge/approvals.py index bf23ffdc..fe87d954 100644 --- a/marge/approvals.py +++ b/marge/approvals.py @@ -1,3 +1,8 @@ +import logging as log +import fnmatch +import re +import shlex + from . import gitlab GET, POST, PUT = gitlab.GET, gitlab.POST, gitlab.PUT @@ -8,16 +13,128 @@ class Approvals(gitlab.Resource): def refetch_info(self): gitlab_version = self._api.version() - if gitlab_version.release >= (9, 2, 2): - approver_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approvals'.format(self) - else: - # GitLab botched the v4 api before 9.2.3 - approver_url = '/projects/{0.project_id}/merge_requests/{0.id}/approvals'.format(self) if gitlab_version.is_ee: - self._info = self._api.call(GET(approver_url)) + self._info = self._api.call(GET(self.get_approvals_url(self))) else: - self._info = dict(self._info, approvals_left=0, approved_by=[]) + self._info = self.get_approvers_ce() + + def get_approvers_ce(self): + """get approvers status using thumbs on merge request + """ + gitlab_version = self._api.version() + + # Gitlab supports approvals in free version after 13.2.0 + approval_api_results = dict(approved_by=[]) + if gitlab_version.release >= (13, 2, 0): + approval_api_results = self._api.call(GET(self.get_approvals_url(self))) + + 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=[]) + + code_owners = self.determine_responsible_owners(owner_file['owners'], self.get_changes_ce()) + + 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 code_owners] + for approver in approval_api_results['approved_by']: + if approver['user']['username'] in code_owners \ + and not any(ex['user']['username'] == approver['user']['username'] for ex in up_votes): + up_votes.append(approver) + + 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([]) + + # Always add global users + if '*' in owners_glob: + owners.update(owners_glob['*']) + + if 'changes' not in changes: + log.info("No changes in merge request!?") + return owners + + for change in changes['changes']: + for glob, users in owners_glob.items(): + test_glob = glob + if glob.endswith('/'): + test_glob += "*" + + if 'new_path' in change and fnmatch.fnmatch(change['new_path'], test_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) + return self._api.call(GET(emoji_url)) + + 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 {"approvals_required": 0, "owners": {}} + + 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) + owner_globs.setdefault(glob, set([])) + + for user in elements: + owner_globs[glob].add(user.strip('@')) + + return {"approvals_required": required, "owners": owner_globs} + + @property + def approvers_string(self): + reviewer_string = "" + + if len(self.codeowners) == 1: + reviewer_string = '@' + self.codeowners.pop() + elif len(self.codeowners) > 1: + reviewer_ats = ["@" + reviewer for reviewer in self.codeowners] + reviewer_string = '{} or {}'.format(', '.join(reviewer_ats[:-1]), reviewer_ats[-1]) + + return reviewer_string + + def get_approvals_url(self, obj): + gitlab_version = self._api.version() + + if gitlab_version.release >= (9, 2, 2): + return '/projects/{0.project_id}/merge_requests/{0.iid}/approvals'.format(obj) + + # GitLab botched the v4 api before 9.2.3 + return '/projects/{0.project_id}/merge_requests/{0.id}/approvals'.format(obj) @property def iid(self): @@ -44,6 +161,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 'codeowners' in self.info: + return self.info['codeowners'] + + return {} + def reapprove(self): """Impersonates the approvers and re-approves the merge_request as them. @@ -55,11 +180,9 @@ def reapprove(self): def approve(self, obj): """Approve an object which can be a merge_request or an approval.""" - if self._api.version().release >= (9, 2, 2): - approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(obj) - else: - # GitLab botched the v4 api before 9.2.3 - approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(obj) + gitlab_version = self._api.version() - for uid in self.approver_ids: - self._api.call(POST(approve_url), sudo=uid) + # Gitlab supports approvals in free version after 13.2.0 + if gitlab_version.is_ee or gitlab_version.release >= (13, 2, 0): + for uid in self.approver_ids: + self._api.call(POST(self.get_approvals_url(obj)), sudo=uid) diff --git a/marge/gitlab.py b/marge/gitlab.py index e025f704..474e7c98 100644 --- a/marge/gitlab.py +++ b/marge/gitlab.py @@ -1,5 +1,6 @@ import json import logging as log +import urllib.parse from collections import namedtuple import requests @@ -10,7 +11,7 @@ def __init__(self, gitlab_url, auth_token): self._auth_token = auth_token self._api_base_url = gitlab_url.rstrip('/') + '/api/v4' - def call(self, command, sudo=None): + def call_raw(self, command, sudo=None): method = command.method url = self._api_base_url + command.endpoint headers = {'PRIVATE-TOKEN': self._auth_token} @@ -35,7 +36,7 @@ def call(self, command, sudo=None): return True # NoContent if response.status_code < 300: - return command.extract(response.json()) if command.extract else response.json() + return response if response.status_code == 304: return False # Not Modified @@ -64,6 +65,40 @@ def other_error(code, msg): raise error(response.status_code, err_message) + def call(self, command, sudo=None): + """call gitlab api and parse response json""" + response = self.call_raw(command, sudo) + return command.extract(response.json()) if command.extract else response.json() + + def repo_file_get(self, project_id, file_path, ref): + """Get file from repository + + The function returns a dict similar to the JSON except + that "encoding" is missing as the content is already + provided decoded as bytes. + + See: https://docs.gitlab.com/ce/api/repository_files.html#get-file-from-repository + """ + file_path = urllib.parse.quote(file_path, safe="") + url = '/projects/{id}/repository/files/{file_path}/raw?ref={ref}' + url = url.format(id=project_id, file_path=file_path, ref=ref) + + try: + result = self.call_raw(GET(url)) + return { + 'file_name': result.headers['X-Gitlab-File-Name'], + 'file_path': result.headers['X-Gitlab-File-Path'], + 'size': result.headers['X-Gitlab-Size'], + 'content_sha256': result.headers['X-Gitlab-Content-Sha256'], + 'blob_id': result.headers['X-Gitlab-Blob-Id'], + 'commit_id': result.headers['X-Gitlab-Commit-Id'], + 'last_commit_id': result.headers['X-Gitlab-Last-Commit-Id'], + 'content': result.text, + 'ref': result.headers['X-Gitlab-Ref'], + } + except NotFound: + return None + def collect_all_pages(self, get_command): result = [] fetch_again, page_no = True, 1 diff --git a/marge/job.py b/marge/job.py index 0646518f..d9c9aca5 100644 --- a/marge/job.py +++ b/marge/job.py @@ -50,10 +50,18 @@ def ensure_mergeable_mr(self, merge_request): approvals = merge_request.fetch_approvals() if not approvals.sufficient: - raise CannotMerge( - 'Insufficient approvals ' - '(have: {0.approver_usernames} missing: {0.approvals_left})'.format(approvals) - ) + message = "" + if not self._api.version().is_ee: + message = ( + 'Insufficient approvals ' + '(have: {0.approver_usernames} missing: {0.approvals_left}). ' + 'Please ask {0.approvers_string} to review').format(approvals) + else: + message = ( + 'Insufficient approvals ' + '(have: {0.approver_usernames} missing: {0.approvals_left}). ').format(approvals) + + raise CannotMerge(message) state = merge_request.state if state not in ('opened', 'reopened', 'locked'): diff --git a/tests/test_approvals.py b/tests/test_approvals.py index 2366bbe2..3a61abc4 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -9,6 +9,127 @@ # testing this here is more convenient from marge.job import CannotMerge, _get_reviewer_names_and_emails +CODEOWNERS = { + "content": """ +# MARGEBOT_MINIMUM_APPROVERS = 2 +# This is an example code owners file, lines starting with a `#` will +# be ignored. + +* @default-codeowner @test-user1 +* @test-user1 @ebert + +unmatched/* @test5 +""" +} + +# 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. + +# 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 + +# 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 +""" # noqa: W605 +} + +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 +204,25 @@ 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': 1, + 'approved_by': [AWARDS[1]], + 'codeowners': {'default-codeowner', 'ebert', 'test-user1'}, } def test_properties(self): @@ -140,3 +269,69 @@ 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() == { + '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'}, + '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() == { + 'approvals_required': 2, + 'owners': {'*': {'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'] == 1 + assert len(result['approved_by']) == 1 + + def test_approvers_string_one(self): + approvals = Approvals(self.api, {'codeowners': {'ebert'}}) + + assert approvals.approvers_string == '@ebert' + + def test_approvers_string_more(self): + approvals = Approvals(self.api, {'codeowners': {'ebert', 'test-user1'}}) + + assert '@ebert' in approvals.approvers_string + assert '@test-user1' in approvals.approvers_string + + def test_approvers_string_empty(self): + approvals = Approvals(self.api, {'codeowners': {}}) + + assert approvals.approvers_string == ''