Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emoji approve based on changes and code owners #254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,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.

### 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 using SSH (what we do)

Assuming you have already got docker installed, the quickest and most minimal
Expand Down
151 changes: 137 additions & 14 deletions marge/approvals.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
qqshfox marked this conversation as resolved.
Show resolved Hide resolved
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should point to the project's default_branch from the API response instead a hardcoded master IMO (you have develop, development, devel, main, etc)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I will have a look at this

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('@'))
Comment on lines +102 to +114
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The matching order for patterns should be ordered and reversed, see https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners (gitlab has the same):

# Order is important; the last matching pattern takes the most
# precedence. When someone opens a pull request that only
# modifies JS files, only @js-owner and not the global
# owner(s) will be requested for a review.

Maybe use https://github.com/sbdchd/codeowners for this instead and any more issues can be solved upstream?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I will have a look at the library


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):
Expand All @@ -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.

Expand All @@ -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)
39 changes: 37 additions & 2 deletions marge/gitlab.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import logging as log
import urllib.parse
from collections import namedtuple

import requests
Expand All @@ -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}
Expand All @@ -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
Expand Down Expand Up @@ -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

Comment on lines +68 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say use https://github.com/python-gitlab/python-gitlab/ for this (disclaimer, I help maintain it so I'm biased here) but I realize the rest of this project doesn't use it and it would take more refactoring for that. But this would be a one-liner if you used a library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't make sense to introduce a new library for this PR, however, it looks like a great idea of another PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, this just needs to get merged :)

def collect_all_pages(self, get_command):
result = []
fetch_again, page_no = True, 1
Expand Down
16 changes: 12 additions & 4 deletions marge/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'):
Expand Down
Loading