Add `git node staging` command #875

ruyadorno · 2024-11-27T17:37:59Z

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:

a21be0294d0 build,win: enable pch for clang-cl windows, build, v8 engine, tools, needs-ci, gyp
c7f82ec9780 doc: fix the return type of outgoingMessage.setHeaders() http, doc, author ready
d8c552a060b doc: edit onboarding guide to clarify when mailmap addition is needed doc, author ready, commit-queue-squash
a5590083cdd node-api: add napi_create_buffer_from_arraybuffer method buffer, c++, node-api, needs-ci, commit-queue-squash
6c836aa97ee meta: bump step-security/harden-runner from 2.9.1 to 2.10.1 meta, dependencies, github_actions
47b6c6748ba meta: bump github/codeql-action from 3.26.6 to 3.26.10 meta, dependencies, github_actions

4 commits that failed to cherry-pick:

71785889c89 lib: prefer logical assignment lib / src, author ready, needs-ci
4988bb549e7 tools: enforce ordering of error codes in errors.md tools, errors, author ready, dont-land-on-v18.x, dont-land-on-v20.x
9f9069d3130 fs: fix linter issue fs, fast-track, author ready, needs-ci, commit-queue-squash
90f56dbad9c module: throw ERR_NO_TYPESCRIPT when compiled without amaro util, module, errors, author ready, needs-ci, strip-types

If no text is provided to `cli.stopSpinner` then it should allow for non-persistent usage of the spinner.

ruyadorno · 2024-11-27T17:50:10Z

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:

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.

richardlau · 2024-11-27T17:52:48Z

lib/staging.js

+    if (this.isLTS) {
+      excludeLabels.push('baking-for-lts');
+      const res = await fetch('https://nodejs.org/dist/index.json');
+      if (!res.ok) throw new Error('Failed to fetch', { cause: res });
+      const [latest] = await res.json();
+      // Assume Current branch matches tag with highest semver value.
+      const latestReleaseLine = semver.coerce(latest.version).major;
+      if (!validNumber(latestReleaseLine)) {
+        throw new Error('Could not determine latest release line');
+      }
+      comparisonBranch = `v${latestReleaseLine}.x`;


The latest current may not be the best thing to compare to. For example, if you were to take the latest v23.x today, that would be v23.3.0 which was only released one week ago and presumably commits would be identified that do not meet the usual "two weeks in a current release" (depending, of course, on when the planned date for the release being prepared is).

Right, we need a better way to handle that 2-weeks delay for sure! That said, for now I think it might still be usable as long as we put enough guardrails in the release docs.

btw I'm def falling for that rn with my current release but given that I postponed it to next week I don't think it's worth going back to remove all the v23.3.0 commits but it goes to say that you're definitely right that this is a concern in the current implementation.

targos · 2024-11-28T01:04:53Z

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.

RafaelGSS · 2024-11-28T02:21:40Z

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?

targos · 2024-11-28T11:32:26Z

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

ruyadorno · 2024-11-28T14:13:23Z

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 · 2024-11-28T14:23:20Z

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

targos · 2024-11-28T15:28:06Z

I agree with everything you said.

aduh95 · 2024-12-03T21:59:08Z

FWIW I'm trying to implement some automation in aduh95/node@38125cf, but it's failing atm

ruyadorno · 2024-12-03T23:33:58Z

FWIW I'm trying to implement some automation in aduh95/node@38125cf, 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?

RafaelGSS · 2024-12-04T14:35:44Z

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

Some nitpicks

components/git/staging.js

RafaelGSS · 2024-12-04T14:40:37Z

components/git/staging.js

+
+const stagingOptions = {
+  backport: {
+    describe: 'The PR ID / number to backport, skip staging commits',


I don't understand why this option is on git node staging?

The idea is that this is a convenience helper for when, let's say, you end up having to remove a commit from the staging branch after figuring out it was breaking CI, then as a releaser you can just run git node staging --backport=<PR_ID> after rebasing / removing the offending commits from the staging branch.

It's part of git node staging more from an implementation convenience point of view since all the internal logic is already there.

Also, it's worth noting that git node staging --backport=<PR_ID> should send an automated comment by default in the original PR, which it's not going to happen in the regular git node staging automation.

RafaelGSS · 2024-12-04T14:43:26Z

lib/staging.js

+
+const validNumber = (n) => !Number.isNaN(n) && n > -1 && n < Infinity;
+
+export class Staging extends Session {


Are we using Session functions somewhere?

We're using warnForMissing & cherryPickInProgress that I remember from the top of my head but there might be some other internal that is used, and opportunities for reusing more of it in the future.

lib/staging.js

ruyadorno · 2024-12-04T16:16:46Z

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 😅

codecov · 2024-12-08T21:20:26Z

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 (6b1349f).
Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/run.js	56.25%	7 Missing ⚠️
lib/cli.js	40.00%	3 Missing ⚠️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aduh95 · 2024-12-12T16:45:54Z

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.

Added a `captureStderr` option to `lib/run.js` `runAsyncBase` method that allows for adding stderr information to thrown errors.

targos · 2024-12-20T07:24:38Z

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

ruyadorno · 2024-12-21T03:34:41Z

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 · 2024-12-21T03:36:04Z

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

@targos I just used it again to help me prepare nodejs/node#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.

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.

chore: stopping spinner with no text skips persist

03936ce

If no text is provided to `cli.stopSpinner` then it should allow for non-persistent usage of the spinner.

ruyadorno mentioned this pull request Nov 27, 2024

tools: add release-helper nodejs/node#51916

Closed

ruyadorno force-pushed the add-git-node-staging-cmd branch from 4dfd7c2 to aa53a75 Compare November 27, 2024 17:41

ruyadorno requested a review from aduh95 November 27, 2024 17:52

richardlau reviewed Nov 27, 2024

View reviewed changes

ruyadorno requested review from RafaelGSS and marco-ippolito November 27, 2024 17:52

RafaelGSS reviewed Dec 4, 2024

View reviewed changes

ruyadorno mentioned this pull request Dec 6, 2024

tools: add a backport queue cron action nodejs/node#56143

Open

ruyadorno force-pushed the add-git-node-staging-cmd branch from aa53a75 to ac8a9b8 Compare December 8, 2024 21:19

ruyadorno force-pushed the add-git-node-staging-cmd branch from ac8a9b8 to b7746a6 Compare December 8, 2024 21:26

ruyadorno added 2 commits December 19, 2024 00:57

chore: add captureStderr option to runAsyncBase

1356262

Added a `captureStderr` option to `lib/run.js` `runAsyncBase` method that allows for adding stderr information to thrown errors.

chore: add lib/run.js unit tests

3afe24f

ruyadorno force-pushed the add-git-node-staging-cmd branch 3 times, most recently from 4ac2739 to 24e8076 Compare December 19, 2024 23:04

ruyadorno force-pushed the add-git-node-staging-cmd branch 2 times, most recently from b909e67 to 639d5fa Compare December 21, 2024 03:15

ruyadorno force-pushed the add-git-node-staging-cmd branch from 639d5fa to 6b1349f Compare December 21, 2024 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `git node staging` command #875

Add `git node staging` command #875

ruyadorno commented Nov 27, 2024 •

edited

Loading

ruyadorno commented Nov 27, 2024 •

edited

Loading

richardlau Nov 27, 2024 •

edited

Loading

ruyadorno Nov 27, 2024

targos commented Nov 28, 2024

RafaelGSS commented Nov 28, 2024

targos commented Nov 28, 2024 •

edited

Loading

ruyadorno commented Nov 28, 2024

ruyadorno commented Nov 28, 2024

targos commented Nov 28, 2024

aduh95 commented Dec 3, 2024

ruyadorno commented Dec 3, 2024

RafaelGSS commented Dec 4, 2024

RafaelGSS left a comment •

edited

Loading

RafaelGSS Dec 4, 2024

ruyadorno Dec 8, 2024

ruyadorno Dec 8, 2024 •

edited

Loading

RafaelGSS Dec 4, 2024

ruyadorno Dec 8, 2024 •

edited

Loading

ruyadorno commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

aduh95 commented Dec 12, 2024

targos commented Dec 20, 2024

ruyadorno commented Dec 21, 2024

ruyadorno commented Dec 21, 2024 •

edited

Loading


		const validNumber = (n) => !Number.isNaN(n) && n > -1 && n < Infinity;

		export class Staging extends Session {

Add git node staging command #875

Are you sure you want to change the base?

Add git node staging command #875

Conversation

ruyadorno commented Nov 27, 2024 • edited Loading

More:

Report

Cherry-pick Report

6 successfully cherry-picked commits:

4 commits that failed to cherry-pick:

ruyadorno commented Nov 27, 2024 • edited Loading

richardlau Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ruyadorno Nov 27, 2024

Choose a reason for hiding this comment

targos commented Nov 28, 2024

RafaelGSS commented Nov 28, 2024

targos commented Nov 28, 2024 • edited Loading

ruyadorno commented Nov 28, 2024

ruyadorno commented Nov 28, 2024

targos commented Nov 28, 2024

aduh95 commented Dec 3, 2024

ruyadorno commented Dec 3, 2024

RafaelGSS commented Dec 4, 2024

RafaelGSS left a comment • edited Loading

Choose a reason for hiding this comment

RafaelGSS Dec 4, 2024

Choose a reason for hiding this comment

ruyadorno Dec 8, 2024

Choose a reason for hiding this comment

ruyadorno Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

RafaelGSS Dec 4, 2024

Choose a reason for hiding this comment

ruyadorno Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

ruyadorno commented Dec 4, 2024 • edited Loading

codecov bot commented Dec 8, 2024 • edited Loading

Codecov Report

aduh95 commented Dec 12, 2024

targos commented Dec 20, 2024

ruyadorno commented Dec 21, 2024

ruyadorno commented Dec 21, 2024 • edited Loading

Add `git node staging` command #875

Add `git node staging` command #875

ruyadorno commented Nov 27, 2024 •

edited

Loading

ruyadorno commented Nov 27, 2024 •

edited

Loading

richardlau Nov 27, 2024 •

edited

Loading

targos commented Nov 28, 2024 •

edited

Loading

RafaelGSS left a comment •

edited

Loading

ruyadorno Dec 8, 2024 •

edited

Loading

ruyadorno Dec 8, 2024 •

edited

Loading

ruyadorno commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 8, 2024 •

edited

Loading

ruyadorno commented Dec 21, 2024 •

edited

Loading