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

Restrict merge to users with write access #67

Closed
Geod24 opened this issue Apr 5, 2020 · 6 comments
Closed

Restrict merge to users with write access #67

Geod24 opened this issue Apr 5, 2020 · 6 comments

Comments

@Geod24
Copy link

Geod24 commented Apr 5, 2020

I've discovered this action recently and would like to use it for the D Programming Language organization.
Currently we have a similar workflow, where users put an auto-merge label and our bot will merge it once the PR is mergeable. We also have auto-merge-squash, etc..., so this action would be a perfect replacement for it.

However we are looking into transitioning our issues from Bugzilla to Github, and as such want to give triage role to users. Currently bugzilla allows anyone to do triage (it's just a matter of registering an email), so I envision we'll be quite liberal with that role. But obviously, we do not want to allow people with triage access to have write access through automerge-action. Is there a way to support this ?

@pascalgn
Copy link
Owner

pascalgn commented Apr 7, 2020

I think there are several ways to do it:

  1. Set branch protection rules in the GitHub repository settings. Mainly, require code reviews for pull requests. If I'm not completely mistaken, GitHub will only count code reviews of users who have write access. Then the PR will only be merged once the required number of users have submitted a (positive) code review for the PR. I think this could best match the workflow of someone "official" needing to give their OK before a merge.

  2. Use the MERGE_FILTER_AUTHOR option. Currently, this only supports 1 author, but it should be very easy to adapt this to support multiple authors. However, I'm not sure how great this will work, probably depending on how often your write-access users change.

  3. Add a new MERGE_FILTER_AUTHOR_ROLE option (or similar) which uses GH API to check if the PR author has write-access to the repository. This could be possible via https://developer.github.com/v3/repos/collaborators/#review-a-users-permission-level


I also want to give a warning regarding GH actions and open source repositories: as discussed in #46, the action will not run in the forks, see this comment: #46 (comment). Maybe you don't need to, but if you require a workflow where actions should also run in the fork repositories, it will probably not work.

@Geod24
Copy link
Author

Geod24 commented Apr 7, 2020

Add a new MERGE_FILTER_AUTHOR_ROLE option (or similar) [...]

That sounds right. I think it's best to avoid duplication between Github access and automerge configuration. For this reason, using MERGE_FILTER_AUTHOR would not really work (doesn't scale). Currently, when someone gets write access, they just get added to a team (there's a team / repository). But some people are admin, some people have direct repository access for historical reasons. So using the role sounds like the most "correct" approach.

if you require a workflow where actions should also run in the fork repositories

Thanks for the information. I was aware of the limitation, and no, we luckily don't need this, nor do I envision we will.

@AArnott
Copy link

AArnott commented Jul 27, 2020

I don't want the PR to have to be authored by someone with write access in order to be auto-completed. I'd like the label to cause any PR to auto-complete. But the label should be automatically removed when the PR is pushed to by someone without write access. That way, I can review any PR, decide to auto-complete it, but if after that point the author pushes to the PR, it guarantees reviewers have an opportunity to review the new changes before merging.

@karfau
Copy link

karfau commented Aug 27, 2020

Github also offers an option to dismiss approvals in case a PR is changes after an approval.
But I'm sure you already know and it's not fine grained enough.

@AArnott
Copy link

AArnott commented Sep 2, 2020

Ya, that doesn't get us what I'm going for. I don't want to dismiss the label when write privileged people push.

FWIW, I've abandoned all efforts to use GitHub Actions for this since github workflows don't trigger by other workflows, so PR checks' completing doesn't cause the Action to reconsider merging the PR. I've switched to a GitHub App which has a lot more flexibility.

@pascalgn
Copy link
Owner

I think the issue from the original post would be solved by having the new MERGE_FILTER_AUTHOR_ROLE option, but I don't need this at the moment and I don't see myself implementing this anytime soon. However, feel very free to work on this and if you create a PR for it, I will have a look at it in a timely manner!

I will close this issue for now. If you have any new information or need any help, feel free to reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants