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
Automate the backports on PR merge to main #27450
base: main
Are you sure you want to change the base?
Conversation
5e75b83
to
6189842
Compare
There is a recent run in my personal repository (only small changes afterwards): https://github.com/ahus1/keycloak/actions/runs/8116038073 (release 23 didn't exist, so it is expected to fail) Here the backport branch reaper did its job: https://github.com/ahus1/keycloak/actions/runs/8115211801 When testing:
|
@ryanemerson - I'd be happy if you could review this when you have the time. I might run another test in my personal repo next week. Thanks! |
6189842
to
2394d04
Compare
Completed my touch-ups and testing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an optional comment, otherwise LGTM 👍
.github/workflows/backport.yaml
Outdated
name: Check Backport | ||
run: | | ||
PR_NUMBER="${{ github.event.pull_request.number }}" | ||
echo "PR: https://github.com/$GITHUB_REPOSITORY/pull/$PR_NUMBER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of recreating the url, you can use: ${{ github.event.pull_request.html_url }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've amended the PR!
2394d04
to
a459f9c
Compare
@stianst - not sure who else to ask for this, so tagging you for a review when you have the time. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had a chance to take a deep look, will try to do that tomorrow or Thursday, but couple things:
- I'd suggest
backport/22.0/<issue-num>
for branch names if possible - Will need to add
backport/**
to branches-ignore for all workflows, as otherwise it will trigger two runs for a backport PR - I'm a bit worried about the reaper could end up deleting other branches if there's a mistake/error in it. Could we perhaps do something like
gh api ... /repos/.../heads/backport/${{ ref }}
so it couldn't delete for instancerelease/22
if something went wrong? Maybe branch protection could also work here, just want to make it somehow fault tolerant and not able to delete main and release branches
Wondering about an slightly alternative approach. We could have a |
a459f9c
to
945aae6
Compare
@stianst - I've updated the PR as you suggested. It is now a dispatch workflow. An example PR created in my repositoy: ahus1#430 Once this is merged, people with write access to this repository can start using/testing it via the GitHub Workflow UI. Next step would updating the bot. UPDATE: after the test, updated the naming pattern for the branch again, and added the missing branch ignores. I added them to all the places where dependabot is also ignored. |
49ead3e
to
ef9d429
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.account.AccountRestServiceTest#updateConsentForClientKeycloak CI - Java Distribution IT (windows-latest - temurin - 19)
|
Closes keycloak#27449 Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
ef9d429
to
4db6ce6
Compare
type: string | ||
default: "24.0" | ||
pullRequest: | ||
description: 'Pull request (just the ID)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be number, PRs have both a number (28087) and an ID (PR_kwDOAKnDVc5qMnnG)
description: 'Pull request (just the ID)' | |
description: 'Pull request number' |
description: 'Release (22.0, 24.0, ...)' | ||
required: true | ||
type: string | ||
default: "24.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a default at all? This is likely to get quickly out of date. Also, can we provide a default if this value is already required
?
runs-on: ubuntu-latest | ||
steps: | ||
- name: Backporting | ||
uses: kiegroup/git-backporting@bce5dd4f9969d47da9cbb6ed06abbf87216afc98 # 4.5.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, should we not be using the existing backport script here directly? If we're going to use an action going forward I'd argue it should be the only way to do a backport to avoid inconsistencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would just use the script and not an action, as I find it easy and quick to do that, but if others prefer a action then I don't see a big deal with having both. Good point about using the same approach both places thouyy
Closes #27449