Conversation
mislav
left a comment
There was a problem hiding this comment.
Thanks for working on this! Looking good, but I would just like to try and keep backwards compatibility.
internal/config/from_env.go
Outdated
| // If GH_HOST is set and there is a valid environment variable | ||
| // token for the host then add it to list. | ||
| if host := os.Getenv(GH_HOST); host != "" { | ||
| if token, _ := AuthTokenFromEnv(host); token != "" { |
There was a problem hiding this comment.
If GH_HOST is set in the environment, would it be a good idea to make it the default host even if GH_ENTERPRISE_TOKEN wasn't set?
There was a problem hiding this comment.
Can you elaborate on what you mean by default host? Are you suggesting that if GH_HOST is set then it should always be returned or are you suggesting that if GH_HOST is set then it should be the only value returned?
There was a problem hiding this comment.
So, what I meant was, I see two conditions here for hostSet.Add(<GH_HOST>) to happen: that GH_HOST is set and that GH_ENTERPRISE_TOKEN is also set. Could we remove the second condition and just do:
if GH_HOST is set
hostSet.Add(<GH_HOST>)
There was a problem hiding this comment.
Yeah that makes sense to me. Updated.
internal/config/from_env.go
Outdated
| // If GH_HOST is set and there is a valid environment variable | ||
| // token for the host then add it to list. | ||
| if host := os.Getenv(GH_HOST); host != "" { | ||
| if token, _ := AuthTokenFromEnv(host); token != "" { |
There was a problem hiding this comment.
So, what I meant was, I see two conditions here for hostSet.Add(<GH_HOST>) to happen: that GH_HOST is set and that GH_ENTERPRISE_TOKEN is also set. Could we remove the second condition and just do:
if GH_HOST is set
hostSet.Add(<GH_HOST>)
This PR changes the behavior of
envConfig.Hosts()to now include in the return setGH_HOSTif it has a corresponding authentication token from one of the numerous environment variable token locations. This solves the case whereauthcommands were displaying that a user was not authenticated despite having bothGH_HOSTand a corresponding authentication token environment variable set.Closes #4576