Conversation
The login name of the authenticated user will be readily available only if authentication info comes from the config file. With other upcoming authentication modes (for example, the GITHUB_TOKEN environment variable), the token is the only piece of information we got, so we would need to additionally query for the login name. Since `issue status` and `pr status` are the only commands that need the name of the authenticated user right now, have those commands explicitly query for the login name. This results in an additional API query, but simplifies Context implementation and future authentication approaches.
If GITHUB_TOKEN is non-blank, it overrides authentication info found in the config file. The config file is, in fact, never consulted.
There was a problem hiding this comment.
Thanks for this! I especially appreciate the attention to detail when printing auth notices.
Just to confirm two things:
- we're introducing username lookup latency for some commands. do you have a sense of what amount of latency it is?
- is there a point anymore of storing a username in the config file anymore?
Thanks for calling this out; I should have pointed this out in the PR description! The latency of the extra request is 180–500ms for me. We can avoid this extra request in
Also a great question! Probably not, but I thought that we best visit this in follow up as we address either #449, #326, or #937. |
probablycorey
left a comment
There was a problem hiding this comment.
I was a little worried about the additional user query at first, but it looks like we only need the user name in our two status commands (and PR can use the magic @me trick)
So it looks good to me.
| fmt.Fprintln(os.Stderr, "or generate a new token for the GITHUB_TOKEN environment variable") | ||
| } else { | ||
| fmt.Fprintln(os.Stderr, "or generate a new token and paste it via `gh config set -h github.com oauth_token MYTOKEN`") | ||
| } |
vilmibm
left a comment
There was a problem hiding this comment.
I feel a little weird about continuing to put username in the config after this but not weird enough to insist we stop and deal with it now.
|
I used Shall I assume, when it comes to github app tokens, using the |
|
If you have a GitHub Apps token, please supply that as |
will there be any difference or things that I should watch out for using GITHUB_TOKEN with other than PAT ? |
In browsers where users would prefer to skip interactive authentication flow, or where such an interaction is not available (e.g. there is no graphical browser), users may set their authentication token via the GITHUB_TOKEN environment variable.
Ref. #297 #773
Combines well with #909
TODO: