Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: use github cli to squash the pr #57675

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tools: use github cli to squash the pr #57675

wants to merge 1 commit into from

Conversation

bjohansebas
Copy link
Member

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

@nodejs-github-bot
Copy link
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 requested changes Mar 31, 2025
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

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
Copy link
Member

lpinca commented Mar 31, 2025

@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
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 reviewed Apr 1, 2025
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

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 bjohansebas requested a review from aduh95 April 1, 2025 20:31
@bjohansebas
Copy link
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
Copy link
Contributor

aduh95 commented Apr 1, 2025

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
Copy link
Member Author

bjohansebas commented Apr 1, 2025 •
edited
Loading

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
Copy link
Contributor

aduh95 commented Apr 1, 2025

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 <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: James M Snell <[email protected]>' --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 <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: James M Snell <[email protected]>' --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
Member Author

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

@aduh95
Copy link
Contributor

aduh95 commented Apr 2, 2025 

$  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
Copy link
Contributor

bnb commented Apr 2, 2025

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
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
Copy link
Contributor

aduh95 commented Apr 3, 2025

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
@aduh95
tools: update repo name for CQ
18d5276 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
@aduh95
tools: update repo name for CQ
6d4dc49 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
@aduh95
tools: update repo name for CQ
2e1eb1a 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
@aduh95
tools: update repo name for CQ
1eb02b4 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
@aduh95
tools: update repo name for CQ
165ee0c 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Apr 5, 2025
@aduh95
tools: update repo name for CQ
a3c9847 
PR-URL: nodejs/node#57675
    Reviewed-By: Luigi Pinca <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Apr 5, 2025

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 approved these changes Apr 6, 2025
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

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
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
Copy link
Contributor

aduh95 commented Apr 6, 2025

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

node/tools/actions/merge.sh

Line 3 in e8633dd

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

@bjohansebas
Copy link
Member Author

It is needed on this line

node/tools/actions/merge.sh

Line 49 in e8633dd

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

aduh95
aduh95 reviewed Apr 7, 2025
View reviewed changes
tools/actions/commit-queue.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
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 :/

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

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca Awaiting requested review from lpinca

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bjohansebas @nodejs-github-bot @lpinca @aduh95 @bnb