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.

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

tools: use GitHub Squash and Merge feature when using CQ #40666

Closed
wants to merge 3 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 30, 2021

GitHub REST API allows us to edit the commit message when using "Squash and Merge" feature, that would allow the CQ to "purple merge" PRs without needing to force-push to the user branch when the PR contains one commit max.

I've tested locally with nodejs/node-auto-test#37, I also tried to test it using the CQ in nodejs/node-auto-test#40 but the CQ is not working there. I think we should land this, and revert if it's broken.

Refs: https://docs.github.com/en/rest/reference/pulls#merge-a-pull-request

@aduh95 aduh95 added the blocked PRs that are blocked by other issues or PRs. label Oct 30, 2021
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 30, 2021
@richardlau
Copy link
Member

I've tested locally with nodejs/node-auto-test#37, I also tried to test it using the CQ in nodejs/node-auto-test#38 but the CQ is not working there.

Maybe the repository check in the workflow?

if: github.repository == 'nodejs/node'

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 31, 2021

I've tested locally with nodejs/node-auto-test#37, I also tried to test it using the CQ in nodejs/node-auto-test#38 but the CQ is not working there.

Maybe the repository check in the workflow?

if: github.repository == 'nodejs/node'

I've tried removing this check, but there are missing repo secrets, I don't know what values I should use: https://github.com/nodejs/node-auto-test/runs/4059202773?check_suite_focus=true

@targos
Copy link
Member

targos commented Oct 31, 2021

Try to replace secrets.GH_USER_TOKEN with the default secrets.GITHUB_TOKEN

@aduh95 aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Nov 1, 2021
@targos
Copy link
Member

targos commented Nov 2, 2021

Wouldn't the script be easier to read and write if we used the github CLI instead of calling the API manually? The CLI is installed on the runners by default

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 2, 2021

Wouldn't the script be easier to read and write if we used the github CLI instead of calling the API manually?

Maybe, maybe not. For merging PRs, the CLI doesn't provide the options to do that (see cli/cli#4645) so we'd have to defer to gh api and that wouldn't help readability much here.
For the other commands, I'd defer to @mmarchini's opinion there might be a good reason she chose curl.

@aduh95

This comment has been minimized.

@aduh95

This comment has been minimized.

@aduh95

This comment has been minimized.

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 4, 2021

The CQ has landed successfully nodejs/node-auto-test#43, including the Landed in… comment. Is there anything in particular I should be testing to have this ready for landing? Or if someone else wants to do their own testing, by all means go try yourself on node-auto-test (I can share credentials for @aduh95-test-account if that helps).

@targos
Copy link
Member

targos commented Nov 5, 2021

@nodejs/actions

@mmarchini
Copy link
Contributor

Ohhh that's an interesting approach to purple merge vs red close a PR without resorting to force-push to the developer's branch.

As for gh vs curl: I think it makes sense as part of a refactor to change everything to use gh instead of curl (even for squashing, for consistency).

@targos wdyt about suggesting a gh api call that uses this same approach on node-core-utils as part of the last steps when merging? That way we can get purple merges even when folks don't use CQ.

@targos
Copy link
Member

targos commented Nov 6, 2021

@targos wdyt about suggesting a gh api call that uses this same approach on node-core-utils as part of the last steps when merging? That way we can get purple merges even when folks don't use CQ.

SGTM, I'll look into it as soon as the script here is refactored to use gh.

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2021

Landed in 19839f8...80b8440

@github-actions github-actions bot closed this Nov 6, 2021
nodejs-github-bot pushed a commit that referenced this pull request Nov 6, 2021
PR-URL: #40666
Refs: https://docs.github.com/en/rest/reference/pulls#merge-a-pull-request
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@aduh95 aduh95 deleted the purple-merge-with-cq branch November 6, 2021 09:10
targos pushed a commit that referenced this pull request Nov 6, 2021
PR-URL: #40666
Refs: https://docs.github.com/en/rest/reference/pulls#merge-a-pull-request
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Nov 8, 2021
BethGriggs pushed a commit that referenced this pull request Nov 25, 2021
PR-URL: #40666
Refs: https://docs.github.com/en/rest/reference/pulls#merge-a-pull-request
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants