Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting by jtmcg · Pull Request #9642 · cli/cli

jtmcg · 2024-09-19T22:23:03Z

Closes #9641

Changes

gh auth login prompting now provides an "other" option in place of "GitHub Enterprise Server"
Tests were added/updated to cover this change
Docs were updated for the gh auth login command to clarify how to use hostnames

To test

Pull down the branch and build using make
Run bin/gh auth login
Notice the change in prompt

Complete the flow to see that it is successful

pkg/cmd/auth/login/login.go

…ompt This change is meant to better support the login flow for other customers besides GitHub Enterprise Server customers that use the same login flow as GHES.

mxie

Left some thoughts on the docs!

pkg/cmd/auth/login/login.go

williammartin

I think this is missing both prompt changes from the A/C:

➜  gh auth login
? Where do you use GitHub? [Use arrows to move, type to filter]
  github.com
> other

? Hostname:

? What account do you want to log into?  [Use arrows to move, type to filter]
? Where do you use GitHub? [Use arrows to move, type to filter]

? GHE Hostname:
? Hostname:

jtmcg · 2024-09-20T20:52:01Z

I think this is missing both prompt changes from the A/C:

➜  gh auth login
? Where do you use GitHub? [Use arrows to move, type to filter]
  github.com
> other

? Hostname:

? What account do you want to log into?  [Use arrows to move, type to filter]
? Where do you use GitHub? [Use arrows to move, type to filter]

? GHE Hostname:
? Hostname:

Totally missed that detail! Thanks for calling them out.

The default authentication mode is a web-based browser flow. After completion, an authentication token will be stored securely in the system credential store. If a credential store is not found or there is an issue using it gh will fallback to writing the token to a plain text file. See `gh auth status` for its stored location. Alternatively, use `--with-token` to pass in a token on standard input. The minimum required scopes for the token are: `repo`, `read:org`, and `gist`. Alternatively, gh will use the authentication token found in environment variables. This method is most suitable for "headless" use of gh such as in automation. See `gh help environment` for more info. To use gh in GitHub Actions, add `GH_TOKEN: ${{ github.token }}` to `env`. The git protocol to use for git operations on this host can be set with `--git-protocol`, or during the interactive prompting. Although login is for a single account on a host, setting the git protocol will take effect for all users on the host. Specifying `ssh` for the git protocol will detect existing SSH keys to upload, prompting to create and upload a new key if one is not found. This can be skipped with `--skip-ssh-key` flag. USAGE gh auth login [flags] FLAGS -p, --git-protocol string The protocol to use for git operations on this host: {ssh|https} -h, --hostname string The hostname of the GitHub instance to authenticate with --insecure-storage Save authentication credentials in plain text instead of credential store -s, --scopes strings Additional authentication scopes to request --skip-ssh-key Skip generate/upload SSH key prompt -w, --web Open a browser to authenticate --with-token Read token from standard input INHERITED FLAGS --help Show help for command EXAMPLES # Start interactive setup $ gh auth login # Authenticate against github.com by reading the token from a file $ gh auth login --with-token < mytoken.txt # Authenticate with specific host $ gh auth login --hostname enterprise.internal LEARN MORE Use `gh <command> <subcommand> --help` for more information about a command. Read the manual at https://cli.github.com/manual Learn about exit codes using `gh help exit-codes` around Tylers-GitHub-MacBook.local

williammartin · 2024-09-23T13:12:07Z

Given I am in an interactive terminal environment
When I run gh auth login
Then I am presented with:

➜  gh auth login
? Where do you use GitHub? [Use arrows to move, type to filter]
  github.com
> other

? Hostname:

➜ ./bin/gh auth login
? Where do you use GitHub?  [Use arrows to move, type to filter]
  GitHub.com
> Other

➜ ./bin/gh auth login
? Where do you use GitHub? Other
? Hostname:

williammartin

Deeply annoying that you can't comment on unmodified lines but I think we should adjust this block

cli/pkg/cmd/auth/login/login.go

Lines 236 to 243 in ccb830c

    
           isEnterprise := hostType == 1 
        
           hostname := ghinstance.Default() 
        
           if isEnterprise { 
        
           	hostname, err = opts.Prompter.InputHostname() 
        
           } 
        
           return hostname, err

Firstly, isEnterprise doesn't capture the intent which is now "other". Secondly, there's no good reason to go lookup the hostname if we're not using it. Thirdly, it's idiomatic to early return in Go. Fourthly, return thing, err in the case where you know err is nil forces the developer to parse any previous conditionals in order to understand that there is no error.

I'd suggest changing this to:

	isGitHubDotCom := hostType == 0
	if isGitHubDotCom {
		return ghinstance.Default(), nil
	}

	return opts.Prompter.InputHostname()

I'm not sure why we bother calling for the default hostname when we know the user chose github.com in the select but it also doesn't really matter.

pkg/cmd/auth/login/login.go

williammartin · 2024-09-23T13:15:20Z

pkg/cmd/auth/login/login_test.go

 	}
 }
+
+func Test_promptForHostname(t *testing.T) {


Interesting test!

…anges The old isEnterprise check no longer makes sense, given the prompter is providing 'other', not 'GitHub Enterprise Server' as its non-GitHub.com option. Additionally, there was an opportunity for cleaning up the code via early returns and the removal of the default hostname lookup if we don't need it.

…og in'

jtmcg · 2024-09-23T16:38:44Z

Given I am in an interactive terminal environment When I run gh auth login Then I am presented with:

➜  gh auth login
? Where do you use GitHub? [Use arrows to move, type to filter]
  github.com
> other

? Hostname:

➜ ./bin/gh auth login
? Where do you use GitHub?  [Use arrows to move, type to filter]
  GitHub.com
> Other

➜ ./bin/gh auth login
? Where do you use GitHub? Other
? Hostname:

Is there a change requested by this comment, @williammartin?

williammartin · 2024-09-23T17:10:36Z

Is there a change requested by this comment, @williammartin?

No, sorry, it was me doing the A/C and then was following up with the review but I didn't comment it as such.

williammartin

LGTM!

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.57.0` -> `v2.58.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.58.0`](https://github.com/cli/cli/releases/tag/v2.58.0): GitHub CLI 2.58.0 [Compare Source](cli/cli@v2.57.0...v2.58.0) #### What's Changed - Better messaging for `attestation verify` custom issuer mismatch error by [@bdehamer](https://github.com/bdehamer) in cli/cli#9616 - Enhance gh repo create docs, fix random cmd link by [@andyfeller](https://github.com/andyfeller) in cli/cli#9630 - Add HasActiveToken method to AuthConfig to refactor auth check for `attestation trusted-root` command by [@BagToad](https://github.com/BagToad) in cli/cli#9635 - Improve the suggested command for creating an issue when an extension doesn't have a binary for your platform by [@timrogers](https://github.com/timrogers) in cli/cli#9608 - Disable auth check for `attestation trusted-root` command by [@bdehamer](https://github.com/bdehamer) in cli/cli#9610 - build(deps): bump github.com/henvic/httpretty from 0.1.3 to 0.1.4 by [@dependabot](https://github.com/dependabot) in cli/cli#9645 - Fix tenant-awareness for `trusted-root` command by [@bdehamer](https://github.com/bdehamer) in cli/cli#9638 - Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting by [@jtmcg](https://github.com/jtmcg) in cli/cli#9642 - build(deps): bump github.com/cpuguy83/go-md2man/v2 from 2.0.4 to 2.0.5 by [@dependabot](https://github.com/dependabot) in cli/cli#9634 - Add `dnf5` instructions to `docs/install_linux.md` by [@its-miroma](https://github.com/its-miroma) in cli/cli#9660 - build(deps): bump github.com/theupdateframework/go-tuf/v2 from 2.0.0 to 2.0.1 by [@dependabot](https://github.com/dependabot) in cli/cli#9688 #### New Contributors - [@its-miroma](https://github.com/its-miroma) made their first contribution in cli/cli#9660 **Full Changelog**: cli/cli@v2.57.0...v2.58.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

jtmcg requested a review from a team as a code owner September 19, 2024 22:23

jtmcg requested a review from BagToad September 19, 2024 22:23

cliAutomation added the external pull request originating outside of the CLI core team label Sep 19, 2024

jtmcg commented Sep 19, 2024

View reviewed changes

pkg/cmd/auth/login/login.go Outdated Show resolved Hide resolved

jtmcg added 2 commits September 19, 2024 15:25

Replace "GitHub Enterprise Server" with "other" in gh auth login pr…

5dd5246

…ompt This change is meant to better support the login flow for other customers besides GitHub Enterprise Server customers that use the same login flow as GHES.

Add documentation explaining how to use hostname for gh auth login

4679097

jtmcg force-pushed the jtmcg/503 branch from d3bcee3 to 4679097 Compare September 19, 2024 22:25

jtmcg added gh-auth relating to the gh auth command and removed external pull request originating outside of the CLI core team gh-auth relating to the gh auth command labels Sep 19, 2024

mxie reviewed Sep 19, 2024

View reviewed changes

pkg/cmd/auth/login/login.go Outdated Show resolved Hide resolved

pkg/cmd/auth/login/login.go Outdated Show resolved Hide resolved

BagToad reviewed Sep 19, 2024

View reviewed changes

pkg/cmd/auth/login/login.go Outdated Show resolved Hide resolved

williammartin requested changes Sep 20, 2024

View reviewed changes

jtmcg added 4 commits September 20, 2024 13:55

Sentence case 'Other' option in hostname prompt

bc587f6

Change prompts for gh auth login to reflect change from GHE to Other

13e916b

Update language on docstring for gh auth login

2059c63

jtmcg requested a review from williammartin September 20, 2024 22:52

williammartin requested changes Sep 23, 2024

View reviewed changes

jtmcg added 2 commits September 23, 2024 09:04

Update docs language to remove possible confusion around 'where you l…

f4af65d

…og in'

jtmcg requested a review from williammartin September 23, 2024 16:37

williammartin approved these changes Sep 23, 2024

View reviewed changes

jtmcg merged commit 9f3f91f into trunk Sep 23, 2024

jtmcg deleted the jtmcg/503 branch September 23, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting#9642

Replace "GitHub Enterprise Server" option with "other" in gh auth login prompting#9642
jtmcg merged 8 commits intotrunkfrom
jtmcg/503

jtmcg commented Sep 19, 2024 •

edited

Loading

Uh oh!

Uh oh!

mxie left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williammartin left a comment

Uh oh!

jtmcg commented Sep 20, 2024

Uh oh!

williammartin commented Sep 23, 2024

Uh oh!

williammartin left a comment •

edited

Loading

Uh oh!

Uh oh!

williammartin Sep 23, 2024

Uh oh!

jtmcg commented Sep 23, 2024

Uh oh!

williammartin commented Sep 23, 2024

Uh oh!

williammartin left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	isEnterprise := hostType == 1

	hostname := ghinstance.Default()
	if isEnterprise {
	hostname, err = opts.Prompter.InputHostname()
	}

	return hostname, err

Conversation

jtmcg commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

To test

Uh oh!

Uh oh!

mxie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

jtmcg commented Sep 20, 2024

Uh oh!

williammartin commented Sep 23, 2024

Uh oh!

williammartin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

williammartin Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

jtmcg commented Sep 23, 2024

Uh oh!

williammartin commented Sep 23, 2024

Uh oh!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtmcg commented Sep 19, 2024 •

edited

Loading

williammartin left a comment •

edited

Loading