-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
doc: mention git-node in the collaborator guide #18960
Conversation
2ae68e9
to
ffe0c38
Compare
COLLABORATOR_GUIDE.md
Outdated
$ git node land $PRID | ||
``` | ||
|
||
If it's the first time you have ever use `node-core-utils`, you will be prompted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove "have" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit addressed
COLLABORATOR_GUIDE.md
Outdated
@@ -569,7 +590,8 @@ commit logs, ensure that they are properly formatted, and add | |||
|
|||
<a name="metadata"></a> | |||
* Modify the original commit message to include additional metadata regarding | |||
the change process. ([`node-core-utils`][] fetches the metadata for you.) | |||
the change process. (the [`git node metadata`][git-node-metadata] command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: . (the
-> . (The
?
Going to attempts to land this as my first PR with git-node :) |
So, I actually discovered something wrong with this PR while trying to land it :D the instructions won't work since all collaborators are required to turn on 2FA nowadays - so the instructions here need to be updated to use the access-token method (rather than the password prompt) as that won't work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will gladly approve again once the guide is updated for 2FA GitHub
@benjamingr I don't think 2FA has much to do with this? ghauth should ask you for the OTP as well if you use the tool to generate the token. |
I have 2FA turned on, this was my correct password, and I did not get a 2FA prompt. For what it's worth I use SSH normally (rather than https) (I'm aware I can fix this by setting the token in the instructions, but I'm interested in making sure new collaborators are successful in using the tool based on the guide and this PR (The tool is really useful)) |
@benjamingr I think that means you have typed your password wrong..? |
I've just tried from another computer and I think it's my (messy) terminal at fault, going to attempt to land again. |
Landed in 1bb92ce Awesome tool @joyeecheung :) |
PR-URL: #18960 Fixes: #18197 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
PR-URL: nodejs#18960 Fixes: nodejs#18197 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
PR-URL: #18960 Fixes: #18197 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
PR-URL: nodejs#18960 Fixes: nodejs#18197 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matheus Marchini <matheus@sthima.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
There's no need to backport this to 8.x or 6.x |
Fixes: #18197
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc