github commit queue feature

[devsnek]

can we get on the signup for this: https://github.com/features/merge-queue/signup

[mcollina]

cc @nodejs/tsc

[targos]

You mean for nodejs/node ? I think we would still not be able to use it for the same reasons we can't use the merge button.

[joyeecheung]

@targos hmm, wait, remind me again why we don't use the merge button? (provided that one updates the commit message properly?)

[targos]

• Squash and merge: No easy way to combine it with our tooling to update and verify the commit message
• Rebase and merge doesn't support autosquash
• Rebase and merge doesn't allow to change the commit messages

[joyeecheung]

oh, right, the merge queue feature of GitHub currently only makes sure that stuff are checked. We need some automation to edit the commit messages also - unless we ask people to edit their commits in the PR branch prior to landing..

[Trott]

unless we ask people to edit their commits in the PR branch prior to landing..

Even then, we'd need to add metadata at landing time.

Don't tell anyone, but I've successfully used the squash and merge button once or twice by adding all the metadata with node-core-utils, then force-pushing to the PR branch. From there, the squash and merge button is usable for single-commit PRs. (It adds the GitHub PR number to the commit message title, but that can be manually removed and isn't a big deal if you don't remove it.) Doing that is more work than following the usual procedure, though. So, not a big win.

[ljharb]

None of the documentation for merge queue seems to describe how the change is merged either - a PR could have 1-3 mechanisms available to it, and could always also be ff-merged from the command line. Is this configurable?

[aduh95]

Don't tell anyone, but I've successfully used the squash and merge button once or twice by adding all the metadata with node-core-utils, then force-pushing to the PR branch. From there, the squash and merge button is usable for single-commit PRs. (It adds the GitHub PR number to the commit message title, but that can be manually removed and isn't a big deal if you don't remove it.) Doing that is more work than following the usual procedure, though. So, not a big win.

Kind of off topic, but I wonder if we could use GitHub CLI tool to do that from the commit-queue for one-commit PRs: https://cli.github.com/manual/gh_pr_merge

[mmarchini]

This seems like a good feature for repositories other than nodejs/node though

[Trott]

Don't tell anyone, but I've successfully used the squash and merge button once or twice by adding all the metadata with node-core-utils, then force-pushing to the PR branch. From there, the squash and merge button is usable for single-commit PRs. (It adds the GitHub PR number to the commit message title, but that can be manually removed and isn't a big deal if you don't remove it.) Doing that is more work than following the usual procedure, though. So, not a big win.

Kind of off topic, but I wonder if we could use GitHub CLI tool to do that from the commit-queue for one-commit PRs: https://cli.github.com/manual/gh_pr_merge

Step one would be to find a way to get the commit-queue to force-push to the PR branch. That will break for certain PRs. (The npm bot PRs, for example, don't allow us to force push to the branch.) But it can fall back to the current push-to-master-and-close-the-PR process that it does currently. For everyone else, the force-push will give us the nice purple merged PRs and improve some of our ability to gather metrics.

And once you have that working, then there's probably no reason not to do it if you have the commit-queue landing multi-commit PRs. And you can use the GitHub CLI tool to merge with the --rebase option.

[aduh95]

Step one would be to find a way to get the commit-queue to force-push to the PR branch. That will break for certain PRs. (The npm bot PRs, for example, don't allow us to force push to the branch.) But it can fall back to the current push-to-master-and-close-the-PR process that it does currently. For everyone else, the force-push will give us the nice purple merged PRs and improve some of our ability to gather metrics.

If we were using the --squash option from GitHub CLI tool, we wouldn't need to force push I think.

gh pr merge <url> --body $COMMIT_MESSAGE_WITH_METADATA --squash

I'll look into that once nodejs/node#40577 has landed (if it lands).

EDIT: I just tried that in nodejs/node-auto-test#34, it unfortunately adds an unwanted first line that consists of PR title (PR number). Too bad that cannot be disabled, otherwise it would be quite useful for our CQ.

[ljharb]

You'd still want to force push tho any time you could, no?

[aduh95]

No, I'd prefer not to force push, it makes the GitHub Actions CI run twice.

[ljharb]

That's a benefit - it's supposed to run on the final commit that will land on master, before it lands on master. This also gives you the opportunity to freshly rebase it before the force push - otherwise anything that lands on master between the last CI run and the merge could cause master to become broken.

Either way, CI on GHA on a public repo is free, what's wrong with running it 20 times, let alone twice?

[aduh95]

Either way, CI on GHA on a public repo is free, what's wrong with running it 20 times, let alone twice?

It's free, but limited, it's not uncommon to have macOS jobs queue for hours when there are a lot of PRs (also, global warming, why run useless CIs?).