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

Replace mergify with GHAs #3346

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Replace mergify with GHAs #3346

merged 7 commits into from
Nov 30, 2023

Conversation

micsza
Copy link
Contributor

@micsza micsza commented Nov 28, 2023

Replacing mergify that gets hard to maintain with GitHub actions with features:

  1. automerge for PRs
  • automerge label is attached if and only if the PR author is softwaremill-ci and exactly one file from the specified list is changed,
  • auto-merge action is performed if and only if there is an automerge label attached and ci and mima jobs complete successfully.
  1. /rebase as PR comment to rebase it.

@micsza micsza force-pushed the replace-mergify-with-ghas branch 2 times, most recently from 3a60a99 to 35bf733 Compare November 28, 2023 10:51
echo "changed_files_num=$N" >> $GITHUB_OUTPUT
- name: Launch labeler
# skip if more than one file changed
if: steps.count-changed-files.outputs.changed_files_num == 1
Copy link
Member

@amorfis amorfis Nov 28, 2023

Choose a reason for hiding this comment

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

Is it always the same file changing? If so, we should check also if this is the one that has changed. We don't want the job to run if it is other file changed do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't see a scenario in which this "modified file swap" thing could happen - would you pls give an example?

Copy link
Member

Choose a reason for hiding this comment

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

Apparently I don't understand something. Doesn't it happen when I modify 1 file - and it's some other file than expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is white list of three files that we consider valid for "automerge" in file labeler.yml. So if steward modifies <> 1 file nothing happens, if it modifies exactly 1 one, then that file will be checked against that white list - if it is on it, then automerge label will attached, otherwise no label.

Copy link
Member

Choose a reason for hiding this comment

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

let's add all this in comments - somebody reading the build file will have no idea to look into labeler.yml :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@@ -103,8 +106,12 @@ jobs:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

mima:
Copy link
Member

@amorfis amorfis Nov 28, 2023

Choose a reason for hiding this comment

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

What is this job for?

Copy link
Member

Choose a reason for hiding this comment

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

Ensures that changes in the core module don't break binary compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

let's add a comment, if it's not clear :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@@ -175,3 +182,38 @@ jobs:
version: "v${{ env.VERSION }}"
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

label:
name: Attach automerge label
Copy link
Member

Choose a reason for hiding this comment

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

can we attach the dependency label? it's later used when creating the release notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, some DRY was needed though.

Copy link
Member

Choose a reason for hiding this comment

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

though now the comment needs updating, as we attach two labels :)

Copy link
Member

Choose a reason for hiding this comment

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

or maybe we can just say this step attaches the labels specified in labelers.yml, but only if the PR changes one file

name: rebase-command
on:
repository_dispatch:
types: [rebase-command]
Copy link
Member

Choose a reason for hiding this comment

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

how are these two wired together? somme comments on how this works and what are the roles of the two workflows would be great :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any comment triggers Slash Comment Dispatch which checks if there is /rebase in the comment body, if yes then it dispatches a rebase command with event type rebase-command which triggers rebase-command workflow that performs the rebase operation. Will add comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added.

name: Slash Command Dispatch
on:
issue_comment:
types: [created]
Copy link
Member

Choose a reason for hiding this comment

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

can we only run this action if the text == /rebase? otherwise we'll get a lot of actions which do nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK, there is no way to trigger workflow based on the content of the issue comment. Eventually we could add a condition for job like if: contains(github.event.comment.body, '/rebase') so that it will be fast-skipped but still the Action space will be polluted with tons of skipped actions triggered, each one triggered by a single comment.

Copy link
Member

Choose a reason for hiding this comment

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

I guess fast-skip is better than nothing, let's try and we'll se if it works fine for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

name: Auto merge
# only for PRs by softwaremill-ci
if: github.event.pull_request.user.login == 'softwaremill-ci'
needs: [ ci, mima, label ]
Copy link
Member

Choose a reason for hiding this comment

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

does a needs=label require that the label was successfully applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly - it requires to wait for label job to complete. The required label (automerge) will be checked anyway inside the action, the needs=label was added just to conform with job dependencies relation - first label, then automerge - and it protects from the (well nearly impossible) scenario when auto-merge job starts when the label job has not yet completed.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok:) yeah I guess I'm missing some info on what's the flow and when various files are used

@micsza micsza force-pushed the replace-mergify-with-ghas branch from 35bf733 to d677f91 Compare November 29, 2023 14:45
@@ -0,0 +1,19 @@
# On any comment, it will look for '/rebase' in the comment body and in case of hit, it dispatches rebase cmd
# with event type 'rebase-command' which triggers 'rebase-command` WF that performs the rebase operation.
Copy link
Member

Choose a reason for hiding this comment

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

WF?

Copy link
Member

Choose a reason for hiding this comment

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

this doesn't explain the relation between rebase-cmd-dispatch and rebase-cmd, I still don't understand what it is

Copy link
Member

@adamw adamw left a comment

Choose a reason for hiding this comment

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

A couple more comments on comments :) Let's merge & see how this works after that - we can test by /rebase-ing a couple of waiting dependency PRs

@micsza micsza merged commit 3cfec95 into master Nov 30, 2023
29 checks passed
@Pask423 Pask423 deleted the replace-mergify-with-ghas branch May 7, 2024 07:12
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.

4 participants