auth logout #1491
auth logout #1491
Conversation
| if showConfirm { | ||
| var keepGoing bool | ||
| err := prompt.SurveyAskOne(&survey.Confirm{ | ||
| Message: fmt.Sprintf("Are you sure you want to log out of %s%s?", hostname, usernameStr), |
There was a problem hiding this comment.
I'm not sure if we need this final confirmation if there was a prompt before this. Two prompts make it look as if the user is potentially doing something really dangerous. The worst thing that can happen to a user that has accidentally logged out is that they will have to go through the OAuth flow again when they want to use gh next, which usually takes under a minute.
So if the user typed out gh auth logout and selected a hostname from a list, I think we can assume that they want to logout from that host without any additional safety nets. Thoughts @ampinsk?
There was a problem hiding this comment.
Chatted with @ampinsk in our sync and for now we'll leave the confirmation in ✨
|
|
||
| mainBuf := bytes.Buffer{} | ||
| hostsBuf := bytes.Buffer{} | ||
| defer config.StubWriteConfig(&mainBuf, &hostsBuf)() |
There was a problem hiding this comment.
Not necessarily something that you have to address here, but I was thinking: now that we have the ability to pass a mock Config when testing a command, and Config is an interface, we could pass our own test stub that conforms to that interface and that verifies that Write was called, but does not actually attempt to write to disk. That way we don't have to rely on package-level function stubbing anymore.
There was a problem hiding this comment.
I'm in favor of reducing package-level function stubbing for sure; here, I like that we're exercising the the part of Write that is taking the yaml tree and actually writing it out. There's just enough logic in there that I'd feel better having coverage for it. I assume we can refactor to have the best of both worlds but I'm not in any rush to do it.
There was a problem hiding this comment.
here, I like that we're exercising the the part of
Writethat is taking the yaml tree and actually writing it out.
For sure! If Write isn't exercised anywhere else in tests, then it's great to keep it active here. 👍
This PR implements
gh auth logout. It's very close to the mockup with one minor difference; whenmultiple accounts are in
hosts.ymlthe user is prompted to select among hostnames as opposed to"github.com" vs "github enterprise".
I chose to error when there's nothing in the hosts config or when a host is specified via
--hostnamethat isn't in the hosts config.
Interactive demo:
Noninteractive demo:
Part of #1413
Depends on #1445