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

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

Closed
wants to merge 1 commit into from

Conversation

derberg
Copy link
Member

@derberg derberg commented Oct 21, 2021

Fixes asyncapi/.github#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-mergelabel 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)

What is missing here

Docs and education. No workflow that would put info on PR about the special comment that people can use. Simple because let us first use it for a couple of weeks to make sure all works well.

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:

Screenshot 2021-10-21 at 12 58 55

@derberg derberg changed the title ci: add merging automation related workflows ci: add workflows to automate merging of human-actor created PRs Oct 21, 2021
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member Author

derberg commented Oct 21, 2021

@jonaslagoni sorry, please move your comment here asyncapi/.github#94

@derberg derberg closed this Oct 21, 2021
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

Successfully merging this pull request may close these issues.

Add global workflow responsible for merging PRs on given conditions
2 participants