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.

Sign up for GitHub

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

Jump to bottom

Merge rights #41

Closed
bkchr opened this issue Sep 25, 2023 · 16 comments · Fixed by #55
Closed

Merge rights #41

bkchr opened this issue Sep 25, 2023 · 16 comments · Fixed by #55
Labels
enhancement New feature or request

Comments

@bkchr
Copy link
Contributor

bkchr commented Sep 25, 2023

We have support for giving approval rights to fellowship members. Now we need to have merge rights. IMO every fellowship member or the block author should be able to write "bot merge" into a pull request comment. If all CI checks are green, the pull request should be merged to master.
@bkchr bkchr added the enhancement New feature or request label Sep 25, 2023
@bkchr
Copy link
Contributor Author

bkchr commented Sep 25, 2023

@rzadp maybe you can help?

@xlc
Copy link
Contributor

xlc commented Sep 25, 2023

It should be possible to have the GH action automatically perform the merge on the approved event. But it maybe a good idea to require an explicitly merge command to trigger the merge.

@bkchr
Copy link
Contributor Author

bkchr commented Sep 25, 2023

It should be possible to have the GH action automatically perform the merge on the approved event.

Yeah, we could havve used mergify for this. However, I don't want to automatically merge when everyone approved, as people may have left some last comments or similar.

@Bullrich
Copy link
Contributor

Why not use a auto merge?

We can do something simple as:

  • When someone comments /bot merge (or your command of preference) it triggers the GitHub action.
  • The GitHub action enables auto-merge in the PR.
  • When all the approvals/checks has passed, it automatically merges it the old fashion way 😃

I would be happy taking this task if you like my proposal.

@bkchr
Copy link
Contributor Author

bkchr commented Sep 26, 2023

  • When someone comments /bot merge (or your command of preference) it triggers the GitHub action.

I like the idea! Just wanted to have the step on deciding "when to enable auto merge" to be manual. Please do it @Bullrich :)

@rzadp
Copy link
Contributor

rzadp commented Sep 26, 2023

  • When someone comments /bot merge (or your command of preference) it triggers the GitHub action.
  • The GitHub action enables auto-merge in the PR.
  • When all the approvals/checks has passed, it automatically merges it the old fashion way 😃

This sounds great to me.

@Bullrich Just FYI it looks like enabling the auto-merge on a PR is not possible with the Github REST API, but it's possible with the GraphQL API.

@xlc
Copy link
Contributor

xlc commented Sep 26, 2023

I guess we will also need to be able to cancel auto merge? But then the permission become a bit unclear.

@bkchr
Copy link
Contributor Author

bkchr commented Sep 26, 2023

I would say we give the same people that can do /bot merge the rights to /bot cancel. If we see that this doesn't work, we can change it in the future? Like only allow to cancel once or whatever. Auto merge should probably also be disabled on any change to the pr.

@Bullrich
Copy link
Contributor

@Bullrich Just FYI it looks like enabling the auto-merge on a PR is not possible with the Github REST API, but it's possible with the GraphQL API.

I looked into it, it should work without any problems, unless that the GraphQL API requires weird permissions (let's hope it doesn't).

I guess we will also need to be able to cancel auto merge? But then the permission become a bit unclear.

@bkchr suggestion of a cancel command sounds good.

Auto merge should probably also be disabled on any change to the pr.

Would it make sense to enable the following branch protection rule?
image

I believe that keeping it active is a good idea as, what you need, is not the user wanting to merge the changes, but the changes (including new changes) to be reviewed. It also makes the process more smooth

I.E.:

  • I make a PR changing something in the readme.
  • I enable auto-merge
  • You correct me because of a typo: change wodr to word.
  • I fix the typo.
  • You approve
  • PR merges.

With the auto disable feature:

  • I make a PR changing something in the readme.
  • I enable auto-merge
  • You correct me because of a typo: change wodr to word.
  • I fix the typo.
  • auto-merge gets disabled.
  • You approve
  • I enable auto-merge
  • PR merges.

In other companies where I worked (and even in the Polkadot-SDK repository) I have seen the auto-merge enabled between changes, as enabling and disabling it constantly only gives more overhead. Checks and reviews still need to be approved for a merge.

@Bullrich
Copy link
Contributor

I have some nice progress.

It is not difficult to achieve 🥳

image

I need to clean up the code, but I'll have a first version ready soon.

https://github.com/paritytech/auto-merge-bot

This was referenced Sep 26, 2023
Closed
Merged
@Bullrich
Copy link
Contributor

Bullrich commented Sep 26, 2023 •
edited
Loading

The application is in development. Find the related tickets here:

@Bullrich
Copy link
Contributor

@bkchr a couple of questions to clarify functionality:

IMO every fellowship member or the block author should be able to write "bot merge" into a pull request comment

I currently set it up to be allowed by users who belong to the org polkadot-fellows or the author of the PR (I assume that is what you meant by block author).

Should I also add in the list of authorized users the users who are part of the fellows? Or being a member of the org suffices the requirement?

@mutantcornholio
Copy link
Contributor

Just to challenge the idea. Maybe allow the author to do GitHub-native automatic merge, just make the fellowship review (via review-bot) the requirement?

Do we want to have two different fellowship steps, where it could be just one?

This was referenced Sep 27, 2023
Closed
Closed
Bullrich added a commit to paritytech/auto-merge-bot that referenced this issue Sep 27, 2023
@Bullrich @rzadp
Added core functionality to auto-merge PRs (#5)
24f7c79 
Created base of the bot, with the ability to comment on a PR and to
enable/disable auto-merge.

You can find a working example in paritytech-stg#1

This resolves #1 

The bot can enable auto-merge, disable it and show the available
commands.

If the user is not a member of the org or the author of the PR, the bot
will not work. This is to stop external parties of enabling/disabling
the bot.

This is a required step to solve polkadot-fellows/runtimes#41

---------

Co-authored-by: Przemek Rzad <[email protected]>
@bkchr
Copy link
Contributor Author

bkchr commented Sep 28, 2023

I assume that is what you meant by block author

Ohh, "block author" 🤦 Too much blockchain for me :P Yes, I meant pr author 🙈

who belong to the org polkadot-fellows or the author of the PR

Not the users of the polkadot-fellows org. Every fellowship member that is registered on chain. Aka use the on chain fellowship list, as we do for approvals. However, for merging every member of the fellowship can issue the command. We just need to have some list to not allow everybody, as they would otherwise troll or whatever.

Just to challenge the idea. Maybe allow the author to do GitHub-native automatic merge, just make the fellowship review (via review-bot) the requirement?

But then only the pr author can do this. And can we even allow external people to the "polkadot-fellows" org to do this?

@mutantcornholio
Copy link
Contributor

And can we even allow external people to the "polkadot-fellows" org to do this?

For some reason, I was sure that that's the case, but now I've doublechecked the docs, and apparently not...
Well, that's that then.

@Bullrich Bullrich mentioned this issue Oct 3, 2023
Merged
Bullrich added a commit to Bullrich/polkadot-fellows-runtimes that referenced this issue Oct 3, 2023
@Bullrich
added github action to enable auto-merge in PRs
b431786 
Added two GitHub actions to enable auto-merge in the repository.

This resolves polkadot-fellows#41

## [Auto-Merge-Bot](https://github.com/paritytech/auto-merge-bot)

This bot allows _public_ members of the organization and the author of the PR to enable/disable auto-merge in a PR. It also has a field for a list of users who are also allowed to trigger the bot. For that we have the second action.
@Bullrich
Copy link
Contributor

Bullrich commented Oct 3, 2023

PR has been created: #55

@bkchr bkchr closed this as completed in #55 Oct 11, 2023
bkchr pushed a commit that referenced this issue Oct 11, 2023
@Bullrich
Added GitHub action to auto-merge (#55)
ee6196c 
* added github action to enable auto-merge in PRs

Added two GitHub actions to enable auto-merge in the repository.

This resolves #41

## [Auto-Merge-Bot](https://github.com/paritytech/auto-merge-bot)

This bot allows _public_ members of the organization and the author of the PR to enable/disable auto-merge in a PR. It also has a field for a list of users who are also allowed to trigger the bot. For that we have the second action.

* added step to allow fellows to trigger the bot

## [Get Fellows Action](https://github.com/paritytech/get-fellows-action)

This action gets the fellows from the chain data and it extracts their GitHub handles. This handles are then input into the `auto-merge-bot` action as a list of users who are allowed to trigger the bot.

* added a pull request template

I have also updated the Pull Request template to mention the existence of this bot to new contributors.

* added mention of the bot in the readme

Updated the readme to contain information about the bot, how to use it, and a link to its documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added GitHub action to auto-merge Bullrich/polkadot-fellows-runtimes
5 participants
@mutantcornholio @xlc @bkchr @Bullrich @rzadp