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

doc: mention git-node in the collaborator guide #18960

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Fixes: #18197

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 23, 2018
$ git node land $PRID
```

If it's the first time you have ever use `node-core-utils`, you will be prompted
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove "have" ?

Copy link
Member

@benjamingr benjamingr left a 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

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

Choose a reason for hiding this comment

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

Nit: . (the -> . (The?

@vsemozhetbyt vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Feb 23, 2018
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 24, 2018
@joyeecheung
Copy link
Member Author

@benjamingr
Copy link
Member

Going to attempts to land this as my first PR with git-node :)

@benjamingr
Copy link
Member

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.

Copy link
Member

@benjamingr benjamingr left a 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

@joyeecheung
Copy link
Member Author

@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.

@joyeecheung
Copy link
Member Author

screen shot 2018-02-26 at 8 32 13 pm

BTW this is how it looks if you have 2FA in your account

@benjamingr
Copy link
Member

benjamingr commented Feb 26, 2018

/Documents/OpenSource/node [master] $ git node land 18960
If this is your first time running this command, follow the instructions to create an access token. If you prefer to create it yourself on Github, see https://github.com/nodejs/node-core-utils/blob/master/README.md.
Your GitHub username: benjamingr
Your GitHub password: ✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔✔

Could not get token: Bad credentials

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))

@joyeecheung
Copy link
Member Author

@benjamingr I think that means you have typed your password wrong..?

https://github.com/rvagg/ghauth/blob/f621230d9b74fe5dc100de45495824787cafde75/ghauth.js#L97-L106

@benjamingr
Copy link
Member

@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.

@benjamingr
Copy link
Member

Landed in 1bb92ce

Awesome tool @joyeecheung :)

@benjamingr benjamingr closed this Feb 26, 2018
benjamingr pushed a commit that referenced this pull request Feb 26, 2018
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Feb 26, 2018
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>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
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>
@addaleax addaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

There's no need to backport this to 8.x or 6.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PSA: custom git command for landing PRs
10 participants