Tracking Issue: commit queue issues and feedback

[mmarchini]

commit-queue can be used to land pull requests. It is still experimental, so when using please remember to check if the Pull Request landed successfully. Please share any failed landings here so we can keep track of issues, our landing process is quite complex and full of edge cases, we'll need to tweak the commit-queue and node-core-utils before we can consider this feature stable.

I'll keep this issue updated with all known issues.

• Commits pushed since last review (buffer: alias UInt ➡️ Uint in buffer methods #34729 (comment))
  • IMO it's fine to request the lander to approve the PR before adding the label
• Missing CI on doc-only changes with successful Actions run (buffer: alias UInt ➡️ Uint in buffer methods #34729 (comment))
• Doesn't work with multiple commits in a PR
• Doesn't work with fixup and squash commits
• Don't land PRs that should be landed manually (manual-land label)
• Can't land DEPXXX (doc: add missing DEP ID for 'new crypto.Certificate()' #34940) (Won't fix, see tools: lint deprecation codes #41992)
• If the newest Jenkins job run is a run that was deleted on Jenkins, the commit queue skips instead of failing
  • But if CI ran on Jenkins and other job was deleted (like CITGM or benchmark), it should succeed
• Should skip if GitHub Actions still running
• Action fails without setting label if local is not sync'ed with remote (which should only happen as a race condition). Should retry instead.
• Land purple
• Don't try to land commits with changes to .github/workflows
• Rewrite in JS
• Try/catch functionality (ensure -failed label is added when something fails)

[mmarchini]

Tried two pull requests, both failed:

• Commits pushed since last review (buffer: alias UInt ➡️ Uint in buffer methods #34729 (comment))
• Missing CI on doc-only changes with successful Actions run (buffer: alias UInt ➡️ Uint in buffer methods #34729 (comment))

[lundibundi]

Simple cases of two last points may be fixed with
nodejs/node-core-utils#473

[lundibundi]

🤔 Anyone knows why but these PRs failed to apply patches:
#34997 (comment)
#34682 (comment)

even though locally it applied successfully with manual git node land. Is three-way-merge somehow disabled for commit-queue?

[mmarchini]

We should add a link to the Action in the bot comment: so we don't have to go chasing it every time it fails: https://github.com/nodejs/node/runs/1102351422?check_suite_focus=true

As for the failures, I'm not entirely sure, it should've worked 🤔

One thing to do when this happens is to run git node land with --yes locally, since that's how it runs on the commit queue. This will ensure we're running the same command. Maybe ncu doesn't go to 3-way-merge with --yes? It should, but there might be a bug.

[mmarchini]

Should wait for GitHub Actions to finish running as well: #35160 (comment)

[aduh95]

Any idea why #34951 is skipped?

[mmarchini]

We have quite a backlog of scheduled Actions for the past hour, so something is up with GitHub apparently:

@aduh95 ncu thinks the CI is still running. Since the label was added four days ago, it's unrelated to the security release lockdown, but I can't check the CI status now because of the lockdown. If I forget to look at it tomorrow after the lockdown is lifted, feel free to ping me.

[targos]

In #33770 (comment), a PR with 3 commits, the bot commented only with the last commit hash, instead of a range.

[mmarchini]

Nice catch, we added the autorebase flag but didn't update the comment generation

[mmarchini]

Found another issue: if a jenkins run was deleted but that's the newest one in the PR, it'll show as "pending" by ncu, which will cause the commit queue to skip (it should fail instead). Also, having a successful CI run should take precedence over having a non-CI already-deleted Jenkins run (like a CITGM or benchmark).

Screenshot might make it easier to understand the issue:

[richardlau]

NCU considers CITGM runs? They're hardly ever green 😞. (It's probably still worth checking one was started, particular for semver-majors.)

[mmarchini]

It checks all Jenkins links it knows how to parse

[richardlau]

Commit queue for #35423 failed but didn't post the reason to the PR (but did update the labels).

[targos]

Commit queue for #35423 failed but didn't post the reason to the PR (but did update the labels).

The logs for that run are here: https://github.com/nodejs/node/runs/1187952829?check_suite_focus=true

It didn't post the reason because the output was apparently too long for jq.
It contains thousands of lines like this:

[devsnek]

i never thought I would see the day jq would be bested

[richardlau]

Commit queue for #35423 failed but didn't post the reason to the PR (but did update the labels).

The logs for that run are here: https://github.com/nodejs/node/runs/1187952829?check_suite_focus=true

It didn't post the reason because the output was apparently too long for jq.
It contains thousands of lines like this:

Looks like that run tried to land more than one PR -- perhaps an edge case where the first failure left things in a bad state?

[BethGriggs]

From #34770 (comment):

Commit queue for #35423 failed but didn't post the reason to the PR (but did update the labels).

The same appears to have happened with #35332 (comment)

[richardlau]

And https://github.com/nodejs/node/runs/1200447439?check_suite_focus=true for #35450.

[richardlau]

Perhaps related to nodejs/node-core-utils#486?

[mmarchini]

Yeah I think 486 is definitely related

[mmarchini]

Fix: #35468

[mmarchini]

Wow that's an interesting edge case. There's probably nothing we can do about CQ being unable to land PRs changing an Action, but it should add the -failed label.

[mmarchini]

Actually, after reading the error again, it should be possible to push changes to .github/workflows assuming that's something we want. We'll need to request an extra scope for the PAT on nodejs/admin though.

[mmarchini]

Added scope to our PAT token, so now we can land PRs with .github/workflow changes.

[aduh95]

The CQ is not working anymore:

fatal: unable to access 'https://github.com/nodejs/node/': The requested URL returned error: 403
Error: Process completed with exit code 128.

https://github.com/nodejs/node/actions/runs/463941833
https://github.com/nodejs/node/actions/runs/463795672
https://github.com/nodejs/node/actions/runs/463659127

Maybe related to the recent token change?

[mmarchini]

That's plausible, I'll take a look.

[mmarchini]

re-generated the token and added it here.

[aduh95]

Same error on recent run (https://github.com/nodejs/node/runs/1655255308?check_suite_focus=true), this must come from somewhere else 🤷‍♂️

[richardlau]

Looks like the request ci label isn't being removed from #36190 resulting in more CI runs being started.

[targos]

The request errors with:

+ curl -sL --request DELETE --url https://api.github.com/repos/nodejs/node/issues/36190/labels/request-ci --header authorization: *** --header content-type: application/json
{
  "message": "Resource not accessible by integration",
  "documentation_url": "https://docs.github.com/rest/reference/issues#remove-a-label-from-an-issue"
}

I temporarily disabled the workflow in https://github.com/nodejs/node/actions?query=workflow%3A%22Auto+Start+CI%22

[richardlau]

I guess this needs someone from the @nodejs/tsc (@mmarchini ?) who can see/generate the tokens to reenable the auto start CI and commit queue workflows.

[mmarchini]

Oh no. I'll try to take a look at it over the weekend.

[richardlau]

Ping @nodejs/tsc again (will also add to the tsc-agenda) regarding the token (which requires an owner, so I guess we could also ping the commcomm) and the broken commit queue. (FTR I'm not sure what it needs/currently has.).

If we're unable to maintain the commit queue we should probably remove it 😞.

[joyeecheung]

What kind of token do we need here?

[mmarchini]

@joyeecheung not sure, the token that we have already should work. It needs permission to edit PRs (to add/remove labels) as well as to commit to the main branch of the repository. It might need other permissions I don't remember from the top of my head.

[mhdawson]

• Was discussed in the meeting today, nobody advocated for removing it so let’s leave
  it for a bit longer/try to fix.
• Those in the meeting would also be ok with removing, if Mary/those involved in maintaining
  the queue decide we should remove it.

Removing agenda tag

[tniessen]

Is it possible that this is related to actions/labeler#50 and actions/first-interaction#10?

[mmarchini]

I'm re-enabling the queue so I can test out the token, if it starts to generate too much spam notifications I'll disable it again.

[mmarchini]

@tniessen that shouldn't be the case since we're using the @nodejs-github-bot account and a schedule trigger (instead of push/PR) to workaround those issues.

[mmarchini]

Updated the token, seems like at some point we lost the repo scope. Re-enabled the commit-queue and request-ci Actions, let's hope they work this time.

[Trott]

I think Should skip if GitHub Actions still running and Land purple can be checked off in the list?

[aduh95]

• Can't land DEPXXX

I think that's a won't fix, we've added a lint rule to validate deprecation numbers that doesn't allow DEPXXX (#41992).