-
Notifications
You must be signed in to change notification settings - Fork 391
Integrate bot for cherry-picking / backporting commits to release branches #1767
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
base: main
Are you sure you want to change the base?
Integrate bot for cherry-picking / backporting commits to release branches #1767
Conversation
…nches Signed-off-by: Karthik Vetrivel <[email protected]>
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.
Made a quick first pass. This is a great start!
.github/workflows/main.yml
Outdated
let branches = []; | ||
|
||
// Get PR number | ||
const prNumber = context.payload.pull_request?.number || context.payload.issue?.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.
Question -- is grabbing a github issue number (the RHS of the conditional statement) necessary / correct here?
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.
Yes, I believe so. I found that when the workflow is triggered by someone commenting / cherry-pick on a PR, GitHub's API doesn't populate context.payload.pull_request. Instead, the PR information is in context.payload.issue. The fallback ensures we get the PR number in both trigger scenarios.
.github/workflows/main.yml
Outdated
const bodyMatches = prBody.matchAll(/\/cherry-pick\s+(release-[\d.]+)/g); | ||
branches.push(...Array.from(bodyMatches, m => m[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.
I am not quite following this. Could you explain what this is doing and why searching the PR body is required? From a quick glance I would assume that searching through the PR comments, as is done below, would suffice.
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.
I added this in the case that the PR author knows upfront which branches need the backport and wants to declare it in the PR description, rather than needing to add a separate comment later. If you don't believe that this is a useful case, I can remove it--let me know
.github/workflows/main.yml
Outdated
|
||
- name: Configure git | ||
run: | | ||
git config user.name "nvidia-bot" |
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.
nit: what about a more descriptive git username? nv-cherrypick-bot
?
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.
Agreed, will update.
EDIT: updated
.github/workflows/main.yml
Outdated
owner: context.repo.owner, | ||
repo: context.repo.repo, | ||
comment_id: context.payload.comment.id, | ||
content: 'eyes' |
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.
👀
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.
Yes! I was following the convention of some other fun workflow bots and add the eyes reaction to the comment
.github/workflows/main.yml
Outdated
execSync(`git checkout -b ${backportBranch} ${targetBranch}`, { stdio: 'inherit' }); | ||
|
||
// Cherry-pick the merge commit | ||
core.info(`Cherry-picking commit ${mergeCommitSha}`); |
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.
@tariq1890 this will cherry-pick the merge commits as opposed to the individual commits that are part of the PR. This differs from our current process of backporting individual commits (not merge commits). Do you have any objections to backporting the merge commits instead?
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.
I strongly prefer the backport of the individual commits over merge commits. It is way more UI friendly when comparing git branches
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.
Can't we retrieve the commits from a PR 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.
I can look into this, it should be possible.
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 @karthikvetrivel !
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.
Signed-off-by: Karthik Vetrivel <[email protected]>
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.
Awesome initiative! left some comments
.github/workflows/main.yml
Outdated
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.
Let's rename the file to a more proper name, like cherrypick.yml or something
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.
Updated!
.github/workflows/main.yml
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
name: Backport merged pull request |
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.
Cherry-Pick, all our workflow files follow a short name approach
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.
Updated.
.github/workflows/main.yml
Outdated
# Run on merged PRs OR on /cherry-pick comments | ||
if: | | ||
(github.event_name == 'pull_request_target' && github.event.pull_request.merged == true) || | ||
(github.event_name == 'issue_comment' && github.event.issue.pull_request && startsWith(github.event.comment.body, '/cherry-pick')) |
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.
Does this mean that the cherry-pick will happen before the PR is approved and merged?
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.
No. The PR must be merged before doing any cherry-picks (lines 118-144). If someone comments /cherry-pick on an unmerged PR, the bot just acknowledges it with 👀 and says it will backport after merge. The actual cherry-pick only happens once the PR is merged.
Generally, It's a convenience add—you can say "this needs to go to release-X.Y" while reviewing, and it'll happen when the PR merges.
.github/workflows/main.yml
Outdated
BRANCHES_JSON: ${{ steps.extract-branches.outputs.result }} | ||
with: | ||
script: | | ||
const branches = JSON.parse(process.env.BRANCHES_JSON); |
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.
This JS script has a lot of lines. It's better to move them to a file instead of keeping them inline in the yaml file
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.
Addressed, moved to seperated Javascript files.
Signed-off-by: Karthik Vetrivel <[email protected]>
32320fc
to
e1ae960
Compare
![]() @tariq1890 Updated the backport bot to cherry-pick individual commits instead of merge commits. Tested with a 2-commit PR to verify it handles multiple commits correctly. |
@@ -0,0 +1,222 @@ | |||
module.exports = async ({ github, context, core }) => { |
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.
Can we have a Licence Header
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.
Updated.
.github/scripts/backport.js
Outdated
|
||
|
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.
Double end of file line
@@ -0,0 +1,42 @@ | |||
module.exports = async ({ github, context, core }) => { |
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.
Licence header
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.
Updated.
.github/workflows/cherrypick.yml
Outdated
|
||
|
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.
Double end of file line
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.
Updated.
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.
Not a JS expert, just a few comments from what I can remember from my early days with JS
.github/workflows/cherrypick.yml
Outdated
|
||
- name: Backport to release branches | ||
id: backport | ||
uses: actions/github-script@v7 |
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.
uses: actions/github-script@v7 | |
uses: actions/github-script@v8 |
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.
Updated.
.github/scripts/backport.js
Outdated
// Create backport branch from target release branch | ||
core.info(`Creating branch ${backportBranch} from ${targetBranch}`); | ||
execSync(`git fetch origin ${targetBranch}:${targetBranch}`, { stdio: 'inherit' }); | ||
execSync(`git checkout -b ${backportBranch} ${targetBranch}`, { stdio: 'inherit' }); |
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.
execSync(`git checkout -b ${backportBranch} ${targetBranch}`, { stdio: 'inherit' }); | |
execSync(`git checkout ${backportBranch} || git checkout -b ${backportBranch} ${targetBranch}`, { stdio: 'inherit' });``` |
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.
Updated.
.github/scripts/extract-branches.js
Outdated
if (context.payload.pull_request?.body) { | ||
const prBody = context.payload.pull_request.body; | ||
// Enforce release-X.Y or release-X.Y.Z | ||
const bodyMatches = prBody.matchAll(/\/cherry-pick\s+(release-\d+\.\d+(?:\.\d+)?)/g); |
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.
const bodyMatches = prBody.matchAll(/\/cherry-pick\s+(release-\d+\.\d+(?:\.\d+)?)/g); | |
// Strict ASCII, anchored; allow X.Y or X.Y.Z | |
const bodyMatches = prBody.matchAll(/^\/cherry-pick\s+(release-\d+\.\d+(?:\.\d+)?)/gmi); |
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.
Updated.
.github/scripts/backport.js
Outdated
1. Review the conflicts in the "Files changed" tab | ||
2. Check out this branch locally: \`git fetch origin ${backportBranch} && git checkout ${backportBranch}\` | ||
3. Resolve conflicts manually | ||
4. Push the resolution: \`git push origin ${backportBranch}\` |
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.
nit: Resolving the conflicts will most likely require a force push:
4. Push the resolution: \`git push origin ${backportBranch}\` | |
4. Push the resolution: \`git push -f origin ${backportBranch}\` |
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.
Updated.
.github/scripts/backport.js
Outdated
git push origin ${backportBranch} | ||
\`\`\` | ||
</details>` | ||
: `🤖 **Automated backport of #${prNumber} to \`${targetBranch}\`** |
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.
nit: I understand that this may be idiomatic, but looking for this :
to differentiate between the two contents is not ideal.
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.
Updated.
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 @karthikvetrivel. This is great.
Is this something that we want to publish as an action at some point, or what options do we have to share this to other repos we own?
Maybe as a follow up: What does the landscape of available actions for doing this look like?
++ To this, since @karthikvetrivel implementation is already in JS, we could have this JS code at k8s-test-infra and from there export it as a GitHub Action so we can reuse in all our repos |
@ArangoGutierrez @elezar thanks for the review! Regarding rollout, I see a few options.
These seems the simplest to me.
This gives repos more flexibility to add custom steps before/after, but our bot needs full job isolation for git operations (script will fail if initial starting state is incorrect), so option 1 seems better.
It seems like option 1 is the best to me but would to leave hear your thoughts. |
.github/scripts/backport.js
Outdated
1. Review the conflicts in the "Files changed" tab | ||
2. Check out this branch locally: \`git fetch origin ${backportBranch} && git checkout ${backportBranch}\` | ||
3. Resolve conflicts manually | ||
4. Push the resolution: \`git push -f origin ${backportBranch}\` |
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.
nit: just to be ultra-safe, I would recommend --force-with-lease
instead of a plain -f
.
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.
Great point, resolved.
.github/scripts/backport.js
Outdated
# Resolve conflicts in your editor | ||
git add . | ||
git commit | ||
git push -f origin ${backportBranch} |
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.
Ditto.
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.
Updated as well.
Signed-off-by: Karthik Vetrivel <[email protected]>
7dbcd48
to
87e9703
Compare
As you recommend option 1, lets go with that. I think we can finish the review here and once we all agree with the implementation, we can talk about a centralized GitHub action so other repos can benefit from this tool |
@ArangoGutierrez Thanks! @elezar @cdesiniotis @tariq1890 what do you think about the current plan? As next steps, we can:
|
Usage
Add a
/cherry-pick
comment to your PR specifying target release branches:When to add the comment:
What Happens
backport
+auto-backport
(clean) orneeds-manual-resolution
(conflicts)Example
Example of a PR where you specify the target release branch:
Example of new PR opened:
Conflict Handling
If cherry-pick has conflicts:
needs-manual-resolution
labelBranch Naming
Release branches must follow the pattern:
release-X.Y
Examples:
release-25.3
,release-24.9.1
,release-23.9
Example