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.

Sign up for GitHub

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

Jump to bottom

new auth flow UX #1548

Merged
merged 17 commits into from
Aug 25, 2020
Merged

new auth flow UX #1548

merged 17 commits into from
Aug 25, 2020

Conversation

vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Aug 18, 2020 •
edited

This PR:

  • removes the "browser auth flow on bad auth" code
  • greets user and asks them to use gh auth login if there is no auth config
  • improves error messaging around 401s, suggesting use of gh auth login
  • improves error messaging when none of a repo's remotes are known by gh

image

closes #1511

@vilmibm vilmibm added this to Backlog 🗒 in The GitHub CLI via automation Aug 18, 2020
@vilmibm vilmibm requested a review from mislav August 18, 2020 23:01
@vilmibm vilmibm moved this from Backlog 🗒 to Needs review 🤔 in The GitHub CLI Aug 18, 2020
@tierninho tierninho self-requested a review August 19, 2020 00:38
The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Aug 19, 2020
tierninho
tierninho approved these changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

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

Ran through the various scenarios, and all login methods were a success. All the new messaging was present as long as new cool graphic. ✅

mislav
mislav suggested changes Aug 19, 2020
View reviewed changes
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.

Love the banner!

My main concern is about having all our bases covered and the fact that with this approach, we have to annotate all leaf commands, meaning that if we add another gh auth subcommand, we have to copy over the annotation. I've proposed a workaround.

Also, as noted in a comment, Cobra pseudo-commands such as help and __complete should never trigger authentication in my view, and also requesting help via -h (where available) and --help.

cmd/gh/main.go Outdated Show resolved Hide resolved
cmd/gh/main.go Outdated
,KMM0;.;:;. dMMMMMMMMK,
:OWMXOOOc dMMMMMWO:
.:kXMMd dMMXk:.
Have a good one~ ,l. .l,
Copy link
Contributor

Choose a reason for hiding this comment

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

“Have a good one” is a colloquialism, and furthermore one that is not always used in good faith. Keeping non-native English speaking people in mind, could we use something simpler, e.g. “happy hacking!”, or nothing at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have a good day?

pkg/cmd/auth/auth.go Show resolved Hide resolved
pkg/cmd/factory/http.go Outdated Show resolved Hide resolved
pkg/cmd/root/root.go Outdated
@@ -98,6 +98,9 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command {
return &cmdutil.FlagError{Err: err}
})

// TODO does this make sense? I'd like people to be able to see usage without an auth message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's a great point. We likely don't want an auth check on commands such as gh, gh pr, gh pr create --help, gh help ..., and similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm okay with gh pr create --help not working for now; there's always gh help and that specific help page isn't going to help much without auth (unlike the root help)

cmd/gh/main.go Outdated
Comment on lines 114 to 129
'oONMMMMMMMMMMMMNOo'
'dXMMMMMMMMMMMMMMMMMMMMXd'
.kMMMMMMMMMMMMMMMMMMMMMMMMMMk.
To authenticate, please lWMMMM:.'ckXKOkkkkOKXkc'.:MMMMWl
run 'gh auth login'. oMMMMMM. .MMMMMMo
;WMMMMMX. .XMMMMMW;
0MMMMMK. .KMMMMM0
You can also set the MMMMMMl lMMMMMM
GITHUB_TOKEN environment MMMMMMd dMMMMMM
variable, if preferred. KMMMMMN' 'NMMMMMK
cMMMMMMNc cNMMMMMMc
xMMNdkXMNkl;,. .,;lkNMMMMMMMx
xWMNx;kWMMMK. .KMMMMMMMMMWx
,KMM0;.;:;. dMMMMMMMMK,
:OWMXOOOc dMMMMMWO:
.:kXMMd dMMXk:.
Copy link
Contributor

Choose a reason for hiding this comment

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

ASCII art 😻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put in a TerminalSize check so as not to print this if the terminal width is too narrow (thanks @tierninho for pointing that out)

The GitHub CLI automation moved this from Needs to be merged 🎉 to Needs review 🤔 Aug 19, 2020
@vilmibm vilmibm requested a review from mislav August 19, 2020 15:23
@AKJK-Internet-Megacorps

This comment has been minimized.

Sign in to view
@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 24, 2020

cc @ampinsk for ascii art sign off >_>

@ampinsk
Copy link
Contributor

ampinsk commented Aug 24, 2020

I hate to be a bummer, but I don't think we should do the ascii art 😞 I'm concerned about how this will show up for users with voice over! I think it could be really confusing, especially on first run.

I would love to include something welcoming like this though, but maybe that could take the form of emoji if we can exclude Windows (where it won't render correctly)?

@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 24, 2020

that's fair, @ampinsk ; for now I've just taken out the ascii art.

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Aug 25, 2020
mislav
mislav approved these changes Aug 25, 2020
View reviewed changes
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 and works great, thank you!

Should alias commands be allowed in no-auth mode?

$ gh alias list
Welcome to GitHub CLI!

To authenticate, please run `gh auth login`.

@vilmibm
Copy link
Contributor Author

vilmibm commented Aug 25, 2020

good catch @mislav , I just missed aliases when I re-did the disable auth check thing

@vilmibm vilmibm merged commit 82c6830 into trunk Aug 25, 2020
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Aug 25, 2020
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Sep 8, 2020
@mislav mislav deleted the auth-check branch October 1, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mislav mislav mislav approved these changes

@tierninho tierninho tierninho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

don't drop into browser flow when no auth detected
5 participants
@vilmibm @AKJK-Internet-Megacorps @ampinsk @mislav @tierninho