ci: add workflows to automate merging of human-actor created PRs

[derberg]

Fixes #31

Why we need it

• AsyncAPI GitHub organization members have write access:
  • security problem when we get at scale (we actually already are)
  • CODEOWNERS restrictions do not work and people that are not in this file but have Wrote on org can approve PRs and merge even though they are not official maintainers
• We have more and more maintainers, there are from time to time mistakes made when PR is merged, that conventional commits spec is wrong, even PR title is correct. Humans make mistakes, bots don't

AsyncAPI GH org changes

Once we are happy with the proposed workflows, this is what will change in the GH org settings:

• Organization members will have only read access
• We invite all TSC members to be organization members and they can also transparently show it on their GH profiles if they want
• We can finally create a TSC GitHub team in organization so we can ping team for important PRs
• All maintainers are no longer outside collaborators but will have write access on "their" repos individually

Change in how maintainers work

You can still work as before
**BUT IT DOESN'T MAKE SENSE 😆 **

• repo maintainers still need write level access to the given repo otherwise CODEOWNERS do not work 😞
• this means you as a maintainer can still manually merge a PR
• you still need to be able to write as there are some edge cases that I can elaborate more on, if this is important for you

Still, I recommend below!

Lukasz's recommended way of working

You are happy with PR, all checks passed? you already approved? DON'T merge manually:

• sometimes we humans forget to make sure the commit message follows semantic versioning as expected.
• sometimes we even forget that we should merge

Just add /ready-to-merge or /rtm comment (can be multiline as in example below)

1. if PR is not draft and not closed, ready-to-merge label will be added
2. automerge for humans workflow will trigger (it triggers also because of other events)
3. if all required checks are green, required approvals in place then the PR will be squash-merged using the PR title (so you do not have to worry about it when merging)

do-not-merge

if all is good and ready-to-merge but you still need to wait, add /do-not-merge or /dnm comment and pr will not get merged

Proof workflows work

Test PR that proves that PR is labeled only when a proper comment is done. You can also see there merging was done, it looks like it is me but only because auto-merge bot in my tests use my token:

Also added support for /help command

Fixes: #78

[jonaslagoni]

I am curious, why is the /rtm command a default use-case? 🤔 As I see it, in normal cases, it is double the work for nothing i.e. both accepting + writing /rtm.

The only use-case I see it being useful is for manual overwriting the behavior if you want multiple people to review PR's. Then something like /wait-for-ready-to-merge and then all review -> /ready-to-merge would be needed (or just label PR wait-for-ready-to-merge). Or just block if multiple people have been requested to review the PR.

Otherwise, me accepting a PR, should IMO be the final acceptance that the PR is ready to be merged and give the PR ready-to-merge label. Less for me to do as a maintainer.

[derberg]

I am curious, why is the /rtm command a default use-case? 🤔 As I see it, in normal cases, it is double the work for nothing i.e. both accepting + writing /rtm.

The only use-case I see it being useful is for manual overwriting the behavior if you want multiple people to review PR's. Then something like /wait-for-ready-to-merge and then all review -> /ready-to-merge would be needed (or just label PR wait-for-ready-to-merge). Or just block if multiple people have been requested to review the PR.

Otherwise, me accepting a PR, should IMO be the final acceptance that the PR is ready to be merged and give the PR ready-to-merge label. Less for me to do as a maintainer.

@jonaslagoni yes there are cases when after approval merge can be done. But how can we guess when it is the case? No, so then you need to know comments like /wait-for-ready-to-merge or just /do-not-merge. What is the difference then between accepting with /rtm or /dnm? 😄 I see one difference, better to forget to say ready to merge than forget to say do not merge, less harmful

[jonaslagoni]

@jonaslagoni yes there are cases when after approval merge can be done. But how can we guess when it is the case? No, so then you need to know comments like /wait-for-ready-to-merge or just /do-not-merge. What is the difference then between accepting with /rtm or /dnm? 😄 I see one difference, better to forget to say ready to merge than forget to say do not merge, less harmful

I would say we can, especially if it is something I have to do each day 😄

What if we created the following checks that determine whether it is automatically merged:

1. PR is not draft
2. PR is not closed
3. All reviewers have accepted the PR
   • This means that if you request a review from 6 people, they need to review the PR before the automatic merge functionality can take over.
4. All required checks are green (maybe all checks, regardless of whether they are required?)
5. PR does not have do-not-merge label

Would this not be suiting and more natural behavior 🤔?

[derberg]

Would this not be suiting and more natural behavior 🤔?

1-4 is already in place

so the question is really only about is it better with do-not-merge to block or ready-to-review to enable. Both anyway have to be added manually as a comment or clicking on labels

so my only argument really is that one of the reasons is to avoid as many human mistakes as possible, and I just afraid of what I wrote: I see one difference, better to forget to say ready to merge than forget to say do not merge, less harmful so basically afraid of situations like crap, I forgot to say "/dnm" and it got merged, crap, I forgot to say "/rtm", not a problem, I can do it now. You know what I mean? 😄

[derberg]

@smoya @jonaslagoni please have another look when you have a moment 😄

[fmvilas]

I think explicitly stating that a PR is "ready to merge" is better than something like "do not merge".

That said, I have fish memory and would not be able to remember all these commands. A comment on every new PR with all the instructions would be helpful. Or a command like /help would trigger the bot to comment with the instructions (less helpful as I still have to remember about /help command haha).

[jonaslagoni]

Got some more questions 😆

Regarding the GH action, automerge-action, I assume that it takes the PR title headline and uses as a commit message which triggers the release. I have noticed that sometimes when I want to merge a PR, the automatic title it gives me is not the current title, but the initial title when the PR was first created. Not sure if the bot has the same "problem", have you tested this? 🤔

Also, have you thought about what happens in the following scenario:

1. PR is created with feat
2. PR is approved by a maintainer
3. PR is marked as ready to merge
4. Evil contributor changes the title of PR to feat!, which does not remove the ready to merge label.
5. PR is merged and a major version is released

Do you think it is possible to set up something where the PR gets "frozen", i.e. mainly title and description cant be changed without needing another review?

[derberg]

@fmvilas actually I think /help is a brilliant idea. We can have it for PRs and Issues and as a response get bet to comment with all available comments on a PR or on an Issue (useful also for @KhudaDad414 and the /good-first-issue javascript moderate)

@jonaslagoni

I have noticed that sometimes when I want to merge a PR, the automatic title it gives me is not the current title, but the initial title when the PR was first create

yeah, saw it too and tried to understand like WHY 😞 but did not figure out what setting is missing. This workflow solves it by taking the latest PR title

Evil contributor changes the title of PR to

🙈 even though I'm not afraid of 👿 this is a possible scenario, yes!
I can easily add a workflow that reacts on:

on:
  pull_request_target:
    types:
      - synchronize
      - edited

and will remove ready-to-merge label.

It will be triggered whenever title/summary is modified or some commits are performed. I'm afraid it might be annoying for us that we need to add the label back and forth, but I don't have a strong opinion, after all we can always just remove the workflow if it is not helpful

[derberg]

@fmvilas /help command added

@jonaslagoni protection from evil added

@smoya other improvements added

[jonaslagoni]

LGTM 🎉 This is gonna be fun 😬

[smoya]

LGTM 🚀

[fmvilas]

I'm approving although I don't like this PR. It's removing my admin superpowers 😢 😛 🚀

[derberg]

@fmvilas @smoya @jonaslagoni @KhudaDad414

folks, I had to push one change that did not change the underlying goal behind the PR. It just solves an edge case situation that we had with wrong handling of bots when working on last release #78.

in short, we were always validating if bot is an actor, but actor is the one who triggers event. So bot can be also an actor in PR that was created by a human. So we simply need to check not actor but PR creator.

not all of you have to approve, just have a look at the syntax if I did not broke anything (I checked in my private repo, but yeah, you never know)

[jonaslagoni]

👍

[KhudaDad414]

👍

[smoya]

LGTM!