search

smarkets / marge-bot

A merge-bot for GitLab
BSD 3-Clause "New" or "Revised" License
701 stars 136 forks source link

Emoji approve based on changes and code owners #254

Open lkm opened 4 years ago

lkm commented 4 years ago

I followed on from the work done by @FlorianLudwig on https://github.com/smarkets/marge-bot/pull/247 and https://github.com/smarkets/marge-bot/issues/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.yml file or add some parsable comment # 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.

lkm commented 4 years ago

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.

FlorianLudwig commented 4 years ago

@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?

lkm commented 4 years ago

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.:

# MARGEBOT_MINIMUM_APPROVERS=1

And perhaps default to either 1 or all appropriate (all matches) code owners? Let me know what you think.

lkm commented 4 years ago

Alright that's been done now. I've also updated the README file to reflect the changes

FlorianLudwig commented 4 years ago

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.:

# MARGEBOT_MINIMUM_APPROVERS=1

And perhaps default to either 1 or all appropriate (all matches) code owners? Let me know what you think.

I was thinking exactly the same.

carno commented 4 years ago

@lkm do You intend to continue work on this pull request?

lkm commented 4 years ago

@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.

sysadmiral commented 4 years ago

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 reviewed-by footer kind of negate the need for this?

lkm commented 3 years ago

@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?

FlorianLudwig commented 3 years ago

@lkm the free version does not parse the CODEOWNERS file

lkm commented 3 years ago

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...?

nejch commented 3 years ago

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.

erikseifert commented 3 years ago

I really want to see this feature. Can i help ?

lkm commented 3 years ago

@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 https://github.com/smarkets/marge-bot/issues/295

I maintain a local fork for my company with these changes and its been running in prod for a year now

nejch commented 3 years ago

@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 :)