Emoji approve based on changes and code owners #254
Conversation
|
Updated to pass linting and also add a comment to the merge request with a list of valid approvers as parsed from codeowners and change set of the MR. |
lkm
force-pushed
the
emoji_approve
branch
2 times, most recently
from
bc8ec6a
to
4b95950
Compare
| # Multiple codeowners can be specified, separated by spaces or tabs | ||
| CODEOWNERS @multiple @code @owners | ||
|
|
||
| # Both usernames or email addresses can be used to match |
There was a problem hiding this comment.
Reading the code only usernames not email addresses work.
There was a problem hiding this comment.
This might fall into the same category as the group comment, I wonder what would be the best way to support emails or whether it's necessary? AFAIK gitlab only supports @username format in the comments so it would take another lookup to the users api to find username for the comment that gets added to the MR. It might be simpler than it sounds, will have a look at it
tests/test_approvals.py
Outdated
|
|
||
| # 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 |
There was a problem hiding this comment.
groups seem not implemented
There was a problem hiding this comment.
Good point! I actually just copied the example CODEOWNERS file from the Gitlab documentation to make sure that the file could be parsed correctly for the tests. While support for groups would be amazing I think that's something that should be done in a follow-up MR. I'll remove those references in the test as it's confusing.
|
@lkm this is pretty awesome! Will try it out today. But already left some comments of my first thoughts. I do prefer the CODEOWNERS file over having a new custom format. Do you have an idea how to handle an approver count setting? |
|
Cheers @FlorianLudwig. I haven't had a chance to test this in prod yet, but will update our marge-bot installation later today and probably update this MR with anything I find. Re the approver count. We need a per project setting for sure, and I agree that it would be undesirable to have the new file, and while I do prefer the structure of a yaml file. I think it would be very simple to add parsing of a line from CODEOWNERS, e.g.:
And perhaps default to either 1 or all appropriate (all matches) code owners? Let me know what you think. |
|
Alright that's been done now. I've also updated the README file to reflect the changes |
I was thinking exactly the same. |
|
@lkm do You intend to continue work on this pull request? |
|
@carno I think it's pretty complete as is. The only thing I would say is that since last release Gitlab supports approvals (only optional not required) in the free version which was my main motivations for this MR, so maybe that can be utilised for a better approach than the awards api. |
|
I would still find this emoji based approach useful on gitlab.com. Currently since marge cannot impersonate approvers on the hosted version if a branch gets rebased the approval is removed and marge-bot cannot do anything. With the emoji approach the "approval" remains on the merge request even after a rebase. This does mean that with the merge request itself there is no longer an actual Gitlab approval attached to the MR but doesn't the |
| return self._api.call(GET(emoji_url)) | ||
|
|
||
| def get_codeowners_ce(self): | ||
| config_file = self._api.repo_file_get(self.project_id, "CODEOWNERS", "master") |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I agree. I will have a look at this
| 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('@')) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for the suggestion. I will have a look at the library
| 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 | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It wouldn't make sense to introduce a new library for this PR, however, it looks like a great idea of another PR
There was a problem hiding this comment.
Yeah, you're right, this just needs to get merged :)
|
@nejch We are using this in production every single day, so I'm interested yes. I do think that sysadmiral's point is still valid regarding approval removal without impersonating. We use our own instance of Gitlab so no problem for us though. I guess it would be possible to support both models side-by-side. Do you know whether gitlab free edition allows anybody to approve a MR or whether it too parses the CODEOWNERS file? |
|
@lkm the free version does not parse the CODEOWNERS file |
|
I've pushed support for using a combination of awards and approvals in CE edition. Haven't tested much in our production environment yet. This PR has been open for such a long time that I'm not sure whether anybody is interested in merging it...? |
Thanks for picking this up again @lkm! Yeah, I wonder as well, there's also @matthiasbalke's PR that's still waiting and would be great. I'll keep poking around this project to see :) Otherwise I had a look and it seems they've actually introduced this upstream for UI rebases: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/489/diffs so maybe it could be extended for local pushes as well, maybe as a project setting. I'm pretty useless at Ruby but I know some people who could guide me perhaps. Then you'd only need to rely on native approvals. IMO the approval feature (in marge) would be a big draw for a lot of people on CE, because gitlab have been pretty vocal about not bringing required approvals to Core, ever. So it might be worth it trying to get native approvals to work for everyone here. |
|
I really want to see this feature. Can i help ? |
|
@erikseifert I would actually suggest that someone fork this project so we can get a proper workflow going, nothing is going to happen here, see #295 I maintain a local fork for my company with these changes and its been running in prod for a year now |
@lkm if you want to rebase and pick this up again, there's some more activity now, and there's also CI running properly now on this repo. Thanks for your patience in any case, I'm trying to get this adopted in our company to compensate for EE features but without resorting to forks :) |
I followed on from the work done by @FlorianLudwig on #247 and #145 and added parsing of changes set to determine who should be able to approve (using the emoji award thumbup).
At the moment I parse CODEOWNERS and require all matched users to have approved. This is probably far from ideal in practice so either we add it as a command line switch or using the suggested
.marge-bot.ymlfile or add some parsable comment #<MIN_REQUIRED:2> to the codeowners file to avoid having to make too many API calls to gitlab. In any case we should also handle cases where available reviewers < required reviewers.