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

PSA: custom git command for landing PRs #18197

Closed
joyeecheung opened this issue Jan 17, 2018 · 8 comments
Closed

PSA: custom git command for landing PRs #18197

joyeecheung opened this issue Jan 17, 2018 · 8 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Jan 17, 2018

node-core-utils v1.9.0 has been released with a custom git command called git-node, which can fetch a PR, check its state, apply the patches, generate the necessary metadata, amend the commit message, and verify the commits.

To land a PR with a single commit you can usually just go with pressing enter until it's complete. To land a PR with multiple commits you can use the command in conjunction with git rebase and friends.

Watch the demos:

  1. Landing multiple commits: https://asciinema.org/a/148627
  2. Landing one commit: https://asciinema.org/a/157445

This has just gone out with little tests (WIP), although I have been using it from my branch for more than two months and it seems to be working pretty well. Would love to have some feedback and see how it works in the wild!

@joyeecheung joyeecheung added the meta Issues and PRs related to the general management of the project. label Jan 17, 2018
@fhinkel
Copy link
Member

fhinkel commented Jan 17, 2018

Thanks! Does it include the checks from core-validate-commit?

@joyeecheung
Copy link
Member Author

joyeecheung commented Jan 17, 2018

@fhinkel "git node land --final" uses core-validate-commit so that has to be installed (still trying to figure out how to bundle it inside)

@benjamingr
Copy link
Member

I think this is definitely worth a @nodejs/collaborators ping - would probably save quite some time :)

@gibfahn
Copy link
Member

gibfahn commented Jan 17, 2018

@fhinkel "git node land --final" uses core-validate-commit so that has to be installed (still trying to figure out how to bundle it inside)

Could we just make core-validate-commit a dependency of node-core-utils (and maybe also a part of the git repo)?

@maclover7
Copy link
Contributor

Very neat tool. I had built my own PR lander tool (https://github.com/maclover7/committer-tools-rb), essentially does the same thing. Really happy to see we are working on automating away lots of stuff these days :)

@ronkorving
Copy link
Contributor

This is great!
Would be good to document this in https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md I reckon?

@mcollina
Copy link
Member

neat! good work!!

@targos
Copy link
Member

targos commented Feb 23, 2018

Great tool! I use it every time now :)

Should we close this issue or keep it open as a reminder to update the collaborator guide?

addaleax pushed a commit to addaleax/node that referenced this issue Feb 26, 2018
PR-URL: nodejs#18960
Fixes: nodejs#18197
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
addaleax pushed a commit that referenced this issue Feb 26, 2018
PR-URL: #18960
Fixes: #18197
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18960
Fixes: nodejs#18197
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants