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

auth status #1495

Merged
merged 8 commits into from
Aug 11, 2020
Merged

auth status #1495

merged 8 commits into from
Aug 11, 2020

Conversation

vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Aug 6, 2020

This PR adds gh auth status.

Bad states:
image

Good state:
image

If GITHUB_TOKEN is specified in the environment, a single status check is done against it:

bad:

~/s/cli (auth-status|✔) $ env GITHUB_TOKEN=REDACTED ghd auth status --hostname tilde.town
X tilde.town: authentication failed

good:

~/s/cli (auth-status|✔) [1] $ env GITHUB_TOKEN=REDACTED ghd auth status
✓ token valid for github.com as vilmibm

I added --hostname; if specified, just that host is checked (either against GITHUB_TOKEN or the token from the config).

Part of #1413

@vilmibm vilmibm added this to Backlog 🗒 in The GitHub CLI via automation Aug 6, 2020
@vilmibm vilmibm moved this from Backlog 🗒 to In progress 🚧 in The GitHub CLI Aug 6, 2020
@vilmibm vilmibm marked this pull request as ready for review August 7, 2020 00:19
@vilmibm vilmibm requested a review from mislav August 7, 2020 00:28
@vilmibm vilmibm moved this from In progress 🚧 to Needs review 🤔 in The GitHub CLI Aug 7, 2020
@vilmibm vilmibm changed the base branch from auth-logout to trunk August 7, 2020 00:29
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

This looks good so far! One tiny but important request: when authentication fails, would this command give you the instructions on how to fix the problem? That's what I imagined the whole point of the command to be.

For example (rough sketch):

# credentials from hosts.yml
$ gh auth status
github.com: authentication failed
The currently stored token has expired. To re-authenticate, run `gh auth login -h github.com`

# credentials from GITHUB_TOKEN
$ gh auth status
github.com: authentication failed
The token in your GITHUB_TOKEN environment variable is invalid. Generate a new token at
https://github.com/settings/tokens and give it "repo", "read:org", and "gist" scopes.

My experience with hub is that often people set GITHUB_TOKEN in their environment and forget about it until the token expires and suddenly none of the commands work anymore. They try to check their credentials in ~/.config/hub and those might be fine, but only later they discover they had an environment variable too. So I'm thinking that our error messages should ideally be explicit about where the token came from in the first place.

Also, would the command list the preferred git clone protocol for each host, or is that planned as follow-up?

pkg/cmd/auth/status/status.go Outdated Show resolved Hide resolved
@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 7, 2020

Here's status with helpful instructions:

image

For missing scopes I added instruction to run gh auth refresh though that doesn't exist yet.

@vilmibm vilmibm requested a review from mislav August 7, 2020 16:48
Copy link
Contributor

@ampinsk ampinsk left a comment

Choose a reason for hiding this comment

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

@vilmibm and I worked through some design changes for the output, and came up with this!

Screen Shot 2020-08-07 at 10 29 17 AM

@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 10, 2020

discussion from sync: have status "print" tokens with *** and accept a flag to show the token.

users may still want to view their tokens in order to authenticate in some other, headless environment. printing the token means they don't have to open the config file or depend on the gh config get oauth_token invocation (since the key name might change at some point).

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Just nits; but overall looks great 🏆

api/client.go Outdated Show resolved Hide resolved
pkg/cmd/auth/status/status_test.go Show resolved Hide resolved
@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 11, 2020

@ampinsk Updated the formatting!

image

(...i ran auth refresh...)

image

@vilmibm vilmibm requested a review from ampinsk August 11, 2020 21:21
@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 11, 2020

NB: I opened a new issue for the token printing behavior instead of tacking that into this already-reviewed PR: #1514

@mislav mislav mentioned this pull request Aug 11, 2020
4 tasks
@ampinsk
Copy link
Contributor

ampinsk commented Aug 11, 2020

@vilmibm looks good!! one thing: those indentations look pretty big, I was thinking similar space to the indentation we have on pr status, which looks like 2 spaces:

github.com
  ✓ Logged in to github.com as vilmibm

@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 11, 2020

@ampinsk done

image

Copy link
Contributor

@ampinsk ampinsk left a comment

Choose a reason for hiding this comment

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

🎉 🎉

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Aug 11, 2020
@vilmibm vilmibm merged commit ec2e8a8 into cli:trunk Aug 11, 2020
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Aug 11, 2020
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

None yet

3 participants