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

Check auth status #32

Closed
heaths opened this issue May 6, 2022 · 3 comments
Closed

Check auth status #32

heaths opened this issue May 6, 2022 · 3 comments

Comments

@heaths
Copy link
Contributor

heaths commented May 6, 2022

If the CLI isn't already authenticated, extensions can fail in various ways depending on what they call. For example, using gh.CurrentRepository() - even in a repo - fails with errors.New("unable to determine current repository, none of the git remotes configured for this repository point to a known GitHub host"). If you specify a repo explicitly, gh.GQLClient() can fail with an internal config.NotFoundError which, given it's in internal, cannot be referenced so might as well be a generic error. To the user, the error simply prints "not found" which isn't actionable. Even the more wordy error mentioned first isn't that actionable, though more useful.

Instead, what about something like gh.IsAuthenticated() bool? It could, for example, call config.Load() and then check the Config.AuthToken for a known host. This would allow extensions to easily check up front if they should prompt the user to authenticate.

Alternatively, expose any auth-related errors in pkg or something so we can do type assertions.

heaths added a commit to heaths/go-gh that referenced this issue May 6, 2022
Fixes cli#32. Also exports error types.
@samcoe
Copy link
Contributor

samcoe commented May 9, 2022

@heaths This does seem like a useful feature to have. I took a quick look at #33 and it looks good so far, but I am going to hold off on a full review for now since I have immediate plans to add lots of functionality for working with gh configs and so the implementation of this feature might look significantly different after that work lands. For now I think we can leave #33 open and I will circle back around to it later. Hope you understand.

@heaths
Copy link
Contributor Author

heaths commented May 9, 2022

Understood. #33 also had a couple options I was expecting to explore. Making the errors public, for example, would at least give callers a way to check for specific errors (apart from taking a fragile dependency on the error string) but if the new work could at least provide a consistent, clear message that the user might not be authenticated that would also be helpful. Then it's just a single error type we'd have to check to give a more actionable message - unless go-gh were to do that itself; however, I understand with the current config implementation there could be other underlying reasons no auth token was found for a given host, for example.

@samcoe
Copy link
Contributor

samcoe commented Jun 21, 2022

@heaths Thanks for your patients here, I got #44 merged in which included auth.TokenForHost function which I think solves this issue. I am going to close for now, please let me know if you think there is some functionality missing.

@samcoe samcoe closed this as completed Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants