Inherit API endpoint configuration from the Codespaces environment #4723
Inherit API endpoint configuration from the Codespaces environment #4723
Conversation
internal/config/from_env.go
Outdated
| return token, GITHUB_ENTERPRISE_TOKEN | ||
| } | ||
|
|
||
| return os.Getenv(GITHUB_TOKEN), GITHUB_TOKEN |
There was a problem hiding this comment.
for some reason we have an explicit test to ensure that
GITHUB_TOKENis not picked up by a non-github.com URL by assuming it's GHES. This PR makes it so that a GHES server would end up reading GITHUB_TOKEN, is there a reason for not doing that?
We intentionally never send GITHUB_TOKEN to non-dotcom instances to avoid ever leaking your github.com token anywhere. This should be reverted. https://github.com/github/pse-assessment-reviews/issues/505#issuecomment-686787043
There was a problem hiding this comment.
@mislav sounds good. The only other solution is that the Codespace will need to send GITHUB_TOKEN under a different name that would be picked up by the CLI (hopefully that is not also a security issue)
internal/codespaces/api/api.go
Outdated
| if apiURL == "" { | ||
| apiURL = githubAPI | ||
| } | ||
| vscsURL := os.Getenv("VSCS_TARGET_URL") |
There was a problem hiding this comment.
I like that all this is now configurable, but this introduces 3 new environment variables:
- GITHUB_SERVER_URL
- GITHUB_API_URL
- VSCS_TARGET_URL
We already supported setting GH_HOST to switch between non-github.com hosts, so adding all of the above feels a bit redundant. Of course, now there is also the https://online.visualstudio.com/api endpoint in addition to the GitHub API, so that needs to be configurable, but I wonder should we still stick with a GH_ prefix for anything that affects the GitHub CLI and name this variable something like GH_VSCS_URL?
There was a problem hiding this comment.
@mislav those env var names are important because they are being created as part of the Codespaces environment and are expected to be automatically picked up by the CLI. Note also that the Codespaces Typescript Extension also picks them up and therefore any changes to the environment variable keys would require changes to both the extension and the CLI and our github/github repository which sends those env vars in the first place.
|
@mislav I am curious why it seems that @cli/codespaces was not pinged on this PR 🤔 |
|
@josebalius not really sure either. But would definitely love a look from your side as well. |
internal/codespaces/api/api.go
Outdated
|
|
||
| // New creates a new API client with the given token and HTTP client. | ||
| func New(token string, httpClient httpClient) *API { | ||
| serverURL := os.Getenv("GITHUB_SERVER_URL") |
There was a problem hiding this comment.
Just a thought, would it be better to pass these in and defer reading from env to the caller? It could make our tests easier if we wanted to use something httptest or something, i know we can set env vars there as well but curious what you think.
There was a problem hiding this comment.
Agreed that it would be best if the API client was agnostic to the environment configuration and instead just accepted a configuration from the caller. In general, lower-level modules should not ever read from the environment directly.
Perhaps the api.API struct can have fields like ServerURL that can be overridden after calling api.New()?
There was a problem hiding this comment.
@mislav @josebalius ok i pulled out the os.Getenv into the caller. Let me know if that looks good.
Perhaps the api.API struct can have fields like ServerURL that can be overridden after calling api.New()?
I typically think it's better that you either create a constructor or have exposed fields. This way it's clear that you only have one way to create the targeted type.
|
@mislav just FYI, I upgraded us to go1.17 to use the new |
josebalius
left a comment
There was a problem hiding this comment.
LGTM, defer to @mislav on the Go version changes
Fixes #4722
One thing to note: is for some reason we have an explicit test to ensure that
GITHUB_TOKENis not picked up by a non-github.com URL by assuming it's GHES. This PR makes it so that a GHES server would end up reading GITHUB_TOKEN, is there a reason for not doing that?Thanks!