Skip to content

tools: use github cli to squash the pr#57675

Closed
bjohansebas wants to merge 1 commit into
nodejs:mainfrom
bjohansebas:gh-title
Closed

tools: use github cli to squash the pr#57675
bjohansebas wants to merge 1 commit into
nodejs:mainfrom
bjohansebas:gh-title

Conversation

@bjohansebas

Copy link
Copy Markdown
Member

Now GitHub CLI allow editing the commit message when squash-merging a PR

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 29, 2025
@bjohansebas bjohansebas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2025

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't let us specify which is the commit head

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 31, 2025
@lpinca

lpinca commented Mar 31, 2025

Copy link
Copy Markdown
Member

@bjohansebas I think that the --match-head-commit option addresses Antoine's comment.

--match-head-commit <SHA>
 Commit SHA that the pull request head must match to allow merge

@bjohansebas

Copy link
Copy Markdown
Member Author

Thanks @lpinca, I didn't really know the block very well.

@bjohansebas bjohansebas requested review from aduh95 and lpinca April 1, 2025 18:06

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you delete the rm output.json line? I don’t think we’re using it anymore

@bjohansebas

Copy link
Copy Markdown
Member Author

Do you know if GH_PROMPT_DISABLED is defined as an environment variable? I'm learning about how Node's CI works, and I don't know if this environment variable is set by default.

@aduh95

aduh95 commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Do you know if GH_PROMPT_DISABLED is defined as an environment variable? I'm learning about how Node's CI works, and I don't know if this environment variable is set by default.

Why do you ask? It doesn't seem that variable is listed on gh pr merge docs: https://cli.github.com/manual/gh_pr_merge

@bjohansebas

bjohansebas commented Apr 1, 2025

Copy link
Copy Markdown
Member Author

Because there may be occasions when the GitHub CLI displays prompts, adding GH_PROMPT_DISABLED can disable them to prevent prompts from appearing.
https://cli.github.com/manual/gh_help_environment

I don't know if it makes sense to add it.

@aduh95

aduh95 commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

I've tried to land a PR with gh pr merge:

$ gh pr merge 57654 --squash --body 'PR-URL: https://github.com/nodejs/node/pull/57654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>' --subject 'doc: clarify `unhandledRejection` events behaviors in process doc' --match-head-commit c9c90f70097af0c445a92045f47a3ba3deb1cd16
X Pull request nodejs/node#57654 is not mergeable: the base branch policy prohibits the merge.
To have the pull request merged after all the requirements have been met, add the `--auto` flag.
To use administrator privileges to immediately merge the pull request, add the `--admin` flag.
$ gh pr merge 57654 --squash --body 'PR-URL: https://github.com/nodejs/node/pull/57654
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>' --subject 'doc: clarify `unhandledRejection` events behaviors in process doc' --match-head-commit c9c90f70097af0c445a92045f47a3ba3deb1cd16 --auto
GraphQL: Pull request Auto merge is not allowed for this repository (enablePullRequestAutoMerge)

It looks like CLI is wrong here, the PR is definitely mergeable, not sure where it's getting that information from

@bjohansebas

Copy link
Copy Markdown
Member Author

Hmm, I've tried the command several times against other repositories. Do you have GitHub CLI updated?

@aduh95

aduh95 commented Apr 2, 2025

Copy link
Copy Markdown
Contributor
$  gh --version
gh version 2.69.0 (1980-01-01)
https://github.com/cli/cli/releases/tag/v2.69.0

It's the latest version, despite being allegedly 45 years old πŸ˜… I've asked @JakobJingleheimer to run the command for me, and it worked (he's using gh version 2.66.1 (2025-01-31))

@bnb

bnb commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

It looks like CLI is wrong here, the PR is definitely mergeable, not sure where it's getting that information from

the output specifically cites the base branch policy prohibits the merge. I'm guessing the token you're using to test this isn't correctly privileged?

@bjohansebas

Copy link
Copy Markdown
Member Author

How could this be unlocked, given that these commands perform similar tasks in principle and there shouldn't be any permission issues, since the token will be the same with my change or with the previous method

@aduh95

aduh95 commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

We should try to set it up on nodejs/node-auto-test and see how it performs. I'll try to do that over the weekend

aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95

aduh95 commented Apr 5, 2025

Copy link
Copy Markdown
Contributor

I tested with https://github.com/nodejs/node-auto-test/actions/runs/14282489948/job/40033645347, the CI sees the same error as I was seeing (the base branch policy prohibits the merge). I tried adding --admin flag, it seemed to fix the issue

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also update tools/actions/merge.sh?

@bjohansebas

Copy link
Copy Markdown
Member Author

it now includes the --admin flag, it's curious that this flag is needed.

@bjohansebas bjohansebas requested a review from aduh95 April 6, 2025 17:36
@aduh95

aduh95 commented Apr 6, 2025

Copy link
Copy Markdown
Contributor

Can you remove the mention of jq as a required deps in

# Requires [gh](https://cli.github.com/), [jq](https://jqlang.github.io), git, and grep. Also awk if you pass a URL.

@bjohansebas

Copy link
Copy Markdown
Member Author

It is needed on this line

if ! commits="$(jq -r 'if .merged then .sha else error("not merged") end' < output)"; then

Comment thread tools/actions/merge.sh
'{merge_method:"squash",commit_title:$title,commit_message:$body,sha:$head}' > output.json
cat output.json
if ! gh api -X PUT "repos/${OWNER}/${REPOSITORY}/pulls/${pr}/merge" --input output.json > output; then
if ! gh pr merge "$pr" --squash --body "$commit_body" --subject "$commit_title" --match-head-commit "$commit_head" --admin > output; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I was saying in https://github.com/nodejs/node/pull/57675/files#r2032046457, this command does not output anything so we can probably not use it for now :/

@bjohansebas

bjohansebas commented Apr 26, 2025

Copy link
Copy Markdown
Member Author

I think the best thing would be to remove the TODO and explain why github cli can't be used

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants