node-core-utils icon indicating copy to clipboard operation
node-core-utils copied to clipboard

Add `git node staging` command

Open ruyadorno opened this issue 1 year ago • 18 comments

Add a new git node staging command that automates cherry-picking commits into staging branches.

It works by cherry-picking all commits that have no conflicts and stopping when finding commits that have conflicts so that the releaser can either manually fix that conflict to continue or skip.

Usage:

Fetches a commit list using branch-diff and automatically cherry-picks / skips commits based on whether or not they land cleanly:

  git node staging

By default the script will add backport-request labels to any skip commit, in order to run in a "dry-run" mode, avoiding the GH cli interactions, you can use the skipGH flag, e.g:

  git node staging --skipGH

After fixing up manual conflicts using git cherry-pick --continue the releaser can resume the cherry-pick queue:

  git node staging --continue

Similarly, after manually skipping a commit using git cherry-pick --skip the releaser can resume the cherry-pick queue using:

  git node staging --skip

If you want to run the script and automatically skip any commit that is unable to land cleanly you can use the --autoSkip flag:

  git node staging --autoSkip

Sets a custom reporter at the end of the automated operation:

  git node staging --reporter=json

Limits to 10 the number of commits to be cherry-picked:

  git node staging --pagination=10

Automates the backport request message, this won't run any of the branch-diff or cherry-pick routines. Useful for when you removed a faulty commit from the branch and want to signal to PR author and collaborators that commit now needs backporting, just use its PR#:

  git node staging --backport=12345

More:

The automate cherry-pick logic also includes local persistency of the ongoing commit list, in case a fatal error happens during the command execution, it's possible to resume after cleaning up the git repo state by running git node staging again.


Report

A report is generated at the end of operations listing information on the commits that were successfully cherry picked and commits that failed to be cherry-picked due to conflicts.

[!IMPORTANT] Below is a short example of how these reports look like. The following report was generated for an operation that was limited using --pagination=10

Cherry-pick Report

6 successfully cherry-picked commits:

4 commits that failed to cherry-pick:

ruyadorno avatar Nov 27 '24 17:11 ruyadorno

When working on the v22 release, I did a progressive run of the automation using the --pagination option to ensure that the tooling was working as intended, as a result I ended up with multiple reports that I'm linking below for future reference:

  1. https://gist.github.com/ruyadorno/29d880a0029c50e6642f2dd1c4d09d1b
  2. https://gist.github.com/ruyadorno/d1813b10d473f1540e008c3ad5cf8769
  3. https://gist.github.com/ruyadorno/2f7d5ecb459f50c71f2e6ed027e942a3
  4. https://gist.github.com/ruyadorno/292b6bdf531e63078b7a5ce9b1d01253

These markdown format reports are way more useful when properly rendered on the GitHub UI or something similar. Right now the command is just printing this result to stdout but as you can imagine this opens the door to having a future GitHub action that either comments or stores this report somewhere, I'm very open to ideas on how to improve that next step of the automation.

ruyadorno avatar Nov 27 '24 17:11 ruyadorno

I understand why you are doing it but I'm afraid of the notification spamming that this generates when there are many conflicts. It's also actually not a good idea to ask for backports on most PRs because landing backports in the wrong order increases the risk of conflicts.

targos avatar Nov 28 '24 01:11 targos

I understand why you are doing it but I'm afraid of the notification spamming that this generates when there are many conflicts. It's also actually not a good idea to ask for backports on most PRs because landing backports in the wrong order increases the risk of conflicts.

I was afraid of it too. Maybe collapse all the messages into a single issue and add the backport-requested label to each one that might reduce the spamming?

RafaelGSS avatar Nov 28 '24 02:11 RafaelGSS

I really like that! It would solve many problems:

  • No more spam
  • Single place to collaborate on backports
  • Encourage doing the backports in the correct order
  • More visibility, by having an open issue instead of comments on merged PRs

targos avatar Nov 28 '24 11:11 targos

Right, I agree that having a single place to manage all the backports has many benefits, I think a simple way to tweak the implementation is to use thegh cli to open a new issue with that markdown report in which the commits that failed to cherry-pick section is the list of commits that need backporting in their expected order. Then the releasers and any other collaborator could work on that.

That said, I still believe that automatically placing the label is a fundamental step that needs to be part of the logic here but we can remove the automatic-commenting to avoid spamming.

ruyadorno avatar Nov 28 '24 14:11 ruyadorno

@targos to reinforce on the idea here, my hope is that this is going to be a much more sane workflow for the releasers.

As anedoctal evidence, for the current release I'm working on I signed off for the day after running the script (which spammed a bunch of comments and added all the labels) and the next day when I woke up in the morning I noticed that @aduh95 jumped in and landed a dozen of the straightforward backports. This happened without any coordination but it does reinforces my believe that if we put a little bit more structure around the process we can have a workflow that is going to be way less demanding on a single releaser.

And of course this new workflow won't prevent the releaser themselves to go through the list of backport-requested commits on their own time and land the straighforward conflict resolutions.

ruyadorno avatar Nov 28 '24 14:11 ruyadorno

I agree with everything you said.

targos avatar Nov 28 '24 15:11 targos

FWIW I'm trying to implement some automation in https://github.com/aduh95/node/commit/38125cf8c8d9c87f6425be92df5045e22c226600, but it's failing atm

aduh95 avatar Dec 03 '24 21:12 aduh95

FWIW I'm trying to implement some automation in https://github.com/aduh95/node/commit/38125cf8c8d9c87f6425be92df5045e22c226600, but it's failing atm

that's awesome as the end goal is to have it running in a GitHub action!

do you believe you can tweak it easily to use git node staging instead?

ruyadorno avatar Dec 03 '24 23:12 ruyadorno

What's missing to have this PR released? Can we land it and create an action to run this once per week to a specific release line? (I suggest v23.x)

RafaelGSS avatar Dec 04 '24 14:12 RafaelGSS

What's missing to have this PR released? Can we land it and create an action to run this once per week to a specific release line? (I suggest v23.x)

I just had a quick chat offline with @aduh95 to make sure we're not duplicating the effort here 😊 but other than that I want to implement the feedback from @targos and tweak git node staging to submit a new issue with the Markdown report instead of posting comments to every single PR that needs backporting.

My stretch goal is to add tests for everything but I'd like to make sure we're committed to using it before putting in all that extra effort 😅

ruyadorno avatar Dec 04 '24 16:12 ruyadorno

Codecov Report

Attention: Patch coverage is 52.38095% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.23%. Comparing base (e3e19b3) to head (dd6402b). Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
lib/run.js 56.25% 7 Missing :warning:
lib/cli.js 40.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
- Coverage   83.08%   81.23%   -1.86%     
==========================================
  Files          37       39       +2     
  Lines        4251     4695     +444     
==========================================
+ Hits         3532     3814     +282     
- Misses        719      881     +162     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 08 '24 21:12 codecov[bot]

As we discussed in the Release meeting, I think that by default, the tool should ask before taking any non-local action (add a label, add a comment), and there should be CLI flags to disable those prompts if e.g. you know you only want to add label and no comment. It should also give the opportunity for the backporter to fix the conflict. This is not a blocking concern for landing this PR, though it would be a blocking concern to document git node staging as the de facto tool for backporting.

aduh95 avatar Dec 12 '24 16:12 aduh95

Please tell me when this is ready for a deep review.

targos avatar Dec 20 '24 07:12 targos

Dropping a quick status update here since I just put in some time on this again over the weekend. It now includes many major changes we discussed here such as:

  • No longer automatically comments on PRs that were unable to land cleanly.
  • No longer automatically skips commits that were unable to land cleanly, by default the tool now works in an interactive mode that exits when finding a commit that is unable to land cleanly and allows you to manually fix it. In cases the releaser is able to fix a commit, running: git cherry-pick --continue & git node staging --continue will resume the cherry-pick to staging session. When the releaser is unable to fix it manually, running: git cherry-pick --skip followed by git node staging --skip will skip that & label that PR with the backport-requested label, optionally the command git node staging --backport <PR_ID> is available to automatically send a comment to that PR, letting the author know that the manual backport is necessary. (This new behavior is inspired by the feedback from @aduh95)
  • The behavior of automatically skipping commits that were unable to land cleanly is now an opt-in setting, it can be used with: git node staging --autoSkip
  • Reports can now be posted automatically to a GitHub issue at the end, the releaser can choose different destinations such as a file or stdout but by default a prompt will ask confirmation to open an issue on GitHub.

ruyadorno avatar Dec 21 '24 03:12 ruyadorno

Please tell me when this is ready for a deep review.

@targos I just used it again to help me prepare https://github.com/nodejs/node/pull/56329 and the new interactive behavior also worked great. The PR itself currently still lacks tests in order to land but it should be in a good enough state for review! As in, I don't plan to change anything drastically at this point.

ruyadorno avatar Dec 21 '24 03:12 ruyadorno

Refs: https://github.com/nodejs/node/issues/57799

RafaelGSS avatar Apr 08 '25 18:04 RafaelGSS

TODO

  • [ ] Proper docs / how to use
  • [ ] Tests
  • [ ] Fix automatic open report on GitHub
  • [ ] Handle private repo refs

ruyadorno avatar Apr 08 '25 21:04 ruyadorno