Honour api_host across the CLI, and give api.Client a flexible request surface - #14104
Conversation
7366aab to
9869df6
Compare
There was a problem hiding this comment.
Pull request overview
This draft PR continues the api_host rollout by moving remaining direct http.Client API call sites onto a shared API request surface, so per-host API routing can be applied consistently across the CLI (including pagination, raw/binary bodies, scope hints, and redirect-sensitive calls). It also adds a purpose-built gateway-based black box harness and acceptance hooks to make api_host bypasses fail loudly.
Changes:
- Introduces a flexible
api.Clientrequest surface (Request,RequestWithContext,DoRequest, per-request headers, endpoint scopes, and redirect control) and migrates multiple command implementations to use it. - Adds
api_hostconfig plumbing (forward lookup and reverse lookup for credential resolution) and updates auth token attachment logic to support gateway hosts. - Adds a containerized gateway + blackhole test harness, plus acceptance test support for gateway runs and additional/updated acceptance scripts.
Show a summary per file
| File | Description |
|---|---|
| script/api-host-gateway/test.sh | Container-internal black box test script exercising routed vs control vs blackhole cases, plus optional acceptance subset runner. |
| script/api-host-gateway/run.sh | Docker entrypoint for the gateway harness; enforces clean working tree and mounts local go.mod replace targets. |
| script/api-host-gateway/README.md | Quickstart documentation for running the gateway harness. |
| script/api-host-gateway/gateway/main.go | Recording TLS reverse proxy that rewrites upstream hostnames out of headers/bodies and logs request evidence. |
| script/api-host-gateway/gateway/main_test.go | Unit tests verifying forwarding, rewriting, logging, and certificate behavior of the gateway proxy. |
| pkg/search/searcher.go | Migrates search requests to api.Client.Request, preserving search-specific error formatting. |
| pkg/cmd/run/view/view.go | Migrates workflow run log ZIP fetching to api.Client.Request while keeping streaming behavior. |
| pkg/cmd/run/view/logs.go | Migrates job log streaming to api.Client.Request with 404-to-“log not found” translation. |
| pkg/cmd/run/download/http.go | Migrates artifact download to api.Client.Request while still streaming ZIP data to disk. |
| pkg/cmd/run/download/http_test.go | Updates tests to account for repo host usage in artifact download requests. |
| pkg/cmd/repo/read-file/read_file_test.go | Updates expected “contents API path” values to be host-relative paths instead of absolute URLs. |
| pkg/cmd/repo/read-file/http.go | Migrates contents/raw file fetches to api.Client.Request and makes contents path host-relative. |
| pkg/cmd/repo/garden/http.go | Migrates commit listing to api.Client.Request and threads host into request helper. |
| pkg/cmd/repo/garden/http_test.go | Adds coverage for success and non-2xx error behavior via api.HTTPError. |
| pkg/cmd/repo/edit/edit.go | Migrates repo topics GET/PUT calls to api.Client.RequestWithContext with explicit headers. |
| pkg/cmd/repo/delete/http.go | Migrates repo delete to api.Client.Request with endpoint scopes and redirect suppression. |
| pkg/cmd/repo/delete/http_test.go | Adds tests pinning delete behavior, scope suggestions, and redirect policy behavior. |
| pkg/cmd/repo/delete/delete_test.go | Strengthens redirect-related stub realism and pins stderr output on error path. |
| pkg/cmd/release/upload/upload.go | Threads repo host into concurrent upload path to support host-aware clients. |
| pkg/cmd/release/shared/upload.go | Refactors upload/delete flows to use api.Client request helpers (DoRequest/RequestWithContext). |
| pkg/cmd/release/shared/upload_test.go | Updates upload retry test harness to use a RoundTripper-backed *http.Client. |
| pkg/cmd/release/shared/fetch.go | Migrates release/ref fetches to api.Client.RequestWithContext and adds 404 sentinel detection. |
| pkg/cmd/release/shared/fetch_test.go | Adds tests for latest release and not-found behavior under the new request/error model. |
| pkg/cmd/release/edit/http.go | Migrates release edit to api.Client.Request while preserving status-dependent behavior. |
| pkg/cmd/release/download/download.go | Switches asset download to api.Client.DoRequest to preserve client redirect policy behavior. |
| pkg/cmd/release/download/download_test.go | Adds integration-style test with real server to pin redirect rewrite behavior. |
| pkg/cmd/release/create/http.go | Migrates “published release exists” probe to api.Client.Request with 404 handling via api.HTTPError. |
| pkg/cmd/release/create/create.go | Threads repo host into concurrent upload path during release creation. |
| pkg/cmd/pr/diff/diff.go | Migrates PR diff fetch to api.Client.Request with proper Accept handling and unexpected-status errors. |
| pkg/cmd/gist/view/view.go | Threads hostname into raw gist file fetch for host-configured clients. |
| pkg/cmd/gist/shared/shared.go | Migrates raw gist file fetch to api.Client.Request (absolute raw URL, host-configured client). |
| pkg/cmd/gist/shared/shared_test.go | Updates gist raw fetch test signature to include hostname. |
| pkg/cmd/gist/edit/edit.go | Threads hostname into raw file fetch for truncated gist files. |
| pkg/cmd/gist/create/create.go | Migrates gist creation to api.Client.Request with endpoint scopes. |
| pkg/cmd/gist/create/create_test.go | Adds scope-suggestion test coverage for gist creation error path. |
| pkg/cmd/extension/manager.go | Threads repo host into extension asset download to keep host-aware request behavior. |
| pkg/cmd/extension/manager_test.go | Updates tests to use absolute API URLs for release assets. |
| pkg/cmd/extension/http.go | Migrates repo existence checks, commit SHA fetch, and asset downloads to api.Client.Request. |
| pkg/cmd/extension/http_test.go | Adds/updates tests for SHA media type, 422 sentinel, octet-stream asset downloads, and unexpected-status behavior. |
| pkg/cmd/auth/shared/oauth_scopes.go | Migrates scope detection calls to api.Client.Request (explicit Authorization header pre-config). |
| pkg/cmd/auth/shared/login_flow.go | Migrates “current viewer login” lookup to api.Client.GraphQL with explicit Authorization header. |
| pkg/cmd/api/http.go | Adds api_host-aware request URL construction for gh api and introduces swapURLHost. |
| pkg/cmd/api/http_test.go | Expands request-building tests for api_host, verbatim path behavior, and Content-Length handling. |
| pkg/cmd/api/api.go | Plumbs per-host api_host into the gh api request path. |
| internal/update/update.go | Migrates update check request to api.Client.RequestWithContext while keeping absolute GitHub API URL behavior. |
| internal/gh/gh.go | Extends auth config interface to support api_host forward and reverse lookups. |
| internal/config/config.go | Implements api_host config accessors and reverse lookup helper for credential resolution. |
| internal/config/auth_config_test.go | Adds coverage for reverse api_host lookup behavior. |
| internal/authflow/flow.go | Implements new auth config interface methods for login flow config shim (no api_host support there). |
| go.mod | Bumps cli/go-gh and updates indirect dependencies (syntax highlighter, regexp, jq/timefmt, timezone, etc.). |
| go.sum | Updates sums corresponding to dependency bumps. |
| docs/api-host.md | Documents api_host behavior, credential resolution, scope, and known gaps. |
| docs/api-host-test-harness.md | Detailed documentation for the gateway harness design, operation, and interpreting failures. |
| api/request_test.go | Adds unit tests for the new api.Client request surface semantics and options. |
| api/queries_repo.go | Migrates repo rename/existence probe to host-relative safeurl paths and Client.Request where appropriate. |
| api/queries_repo_test.go | Adds test coverage for “unexpected 2xx” existence probe behavior. |
| api/http_client.go | Adds reverse api_host mapping fallback when attaching auth tokens to gateway-hosted requests. |
| api/http_client_test.go | Adds test cases ensuring gateway host requests receive the canonical host token, without hijacking real credentials. |
| api/client.go | Introduces request options + new request methods (Request*, DoRequest) and UnexpectedStatusError. |
| api/client_test.go | Adds tests covering redirect suppression behavior and UnexpectedStatusError. |
| acceptance/testdata/search/search-issues.txtar | Increases indexing wait to reduce search flake during acceptance. |
| acceptance/testdata/gist/gist-edit-rename-list.txtar | Adds new gist acceptance script covering edit/add/rename/list behavior with required sleeps. |
| acceptance/testdata/gist/gist-create-view-delete.txtar | Adds gist acceptance script covering create/view/raw/delete flows. |
| acceptance/scriptfilter_unit_test.go | Adds unit tests for new multi-script filtering support. |
| acceptance/scriptfilter_test.go | Adds filter parsing + script selection helpers used by acceptance runner. |
| acceptance/acceptance_test.go | Adds gist test group, supports multi-script filtering, supports gateway runs via hosts.yml + SSL_CERT_FILE propagation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 65/66 changed files
- Comments generated: 1
- Review effort level: Lite
| // swapURLHost replaces the host component of rawURL with newHost, preserving | ||
| // scheme, port (if already present in rawURL), path, and query unchanged. |
There was a problem hiding this comment.
I'm just flagging this, but I don't think we need to change anything here unless there's need for it.
Agreed with
. Assigning u.Host clears any port numbers in the original URL. Copilot suggests we keep it as is and only update the godoc. The problem with that is we'll replace any original port number with the one in the newHost. I think it's almost okay because the original URL is meant to point at GitHub API hosts (e.g. api.github.com) and it's so rare that a production API exposes a non-default 443/80 port publicly.
The other approach, which is to preserve the port can also okay but there's a caveat, which is we should first store the original port number, then replace the host, and if there's no port number, then we replace it. This is because the newHost can be configured with a trailing port number.
|
|
||
| type tokenGetter interface { | ||
| ActiveToken(string) (string, string) | ||
| HostForAPIHost(string) (string, bool) |
There was a problem hiding this comment.
In a later commit on a different branch this moves to a different interface, it shouldn't have been overloaded on tokenGetter.
| $ GH_APIHOST_ACCEPTANCE=yes GH_APIHOST_ORG=my-org script/api-host-gateway/run.sh | ||
| ``` | ||
|
|
||
| This creates real repositories and can take up to 45 minutes. It prompts for a |
There was a problem hiding this comment.
This is not correct, it takes just a few minutes to run, it's just the timeout is set high. A full acceptance suite may take 15-20 minutes though, but that's not what is running in this subset.
babakks
left a comment
There was a problem hiding this comment.
Thanks for this, @williammartin! 🙏
First off, real appreciation for the testing plan. The isolated network approach, blackholing github.com inside the container so a request that ignores api_host cannot connect at all, is a genuinely clever way to make routing falsifiable rather than something we hope is true. The per name, one script per invocation recording made it easy to reason about exactly which call site each change fixed.
I went through all of the changes, including the mechanical ones, and they read as reasonable and solid to me. There may be edge cases lurking, but if so they are the hard to find kind rather than anything obvious.
I have left a number of inline suggestions on the diff. Anything I considered a nitpick is marked as such.
A few overall points beyond the inline notes:
-
The
RequestOptionapproach is lovely. It is both an idiomatic way to introduce optional arguments and mutations, and a nicely centralised place for call sites to shape their interaction with the API client. I expect this to age well. -
api.UnexpectedStatusErrorchanges the error text on unexpected 2xx responses. For example, at the body decoding call sites, aHTTP 204(if ever) would previously have surfaced as a JSON unmarshal or EOF error, and at header reading sites likeGetScopesa non 200 used to go throughHandleHTTPError. Both now yield the uniformUnexpectedStatusErrormessage. This is not quite real, but you get my point. Anyawy, users should not be relying on error text, so I think we are fine here. Just calling it out. -
[Blocker] The new request methods need to be taught to the
SafeURLPathConstruction.qlCodeQL query.RequestandRequestWithContextaccept a path but are not covered byisHttpUrlArgument, so a hand built URL passed to them would slip past the safeurl encoding check. The path argument index differs:Requestcan join the existing arg 2 branch alongsideRESTandRESTWithNext, whileRequestWithContextneeds its own branch at arg 3. I built a database locally and ran a modified query to confirm, which turned up two things worth handling as part of this: addingRequestWithContextalso flagsapi/client.goitself, whereRequestdelegates to it with thepparameter, so the query wants to exclude theapipackage; and the PR already introduces a real newly flagged site inpkg/cmd/auth/shared/oauth_scopes.go, which callsRequest(hostname, GET, "", nil, ...)with an empty string path. That one is benign since there is no variable path component, but it still needs either a safeurl wrapping or an allowlist to keep the guard green. I would like to see the query updated and this site reconciled before we merge. -
[Blocker?] Please document the new request surface in
AGENTS.md. The "API Patterns" section currently only mentionsGraphQLandREST. It would help future contributors to note when to reach for each of the new methods:RequestandRequestWithContextfor when you need the raw response, such as a streaming or non JSON body, a response header, or the status code of a success, andDoRequestfor a caller built request that must control something the others cannot express, such asContentLengthandGetBodyon an upload or aCheckRedirecton the client. Worth restating the guidance to prefer relative paths so host andapi_hostresolution applies, since absolute URLs bypass it.
Really nice piece of work overall. Enjoy the vacation! 🍻
| // swapURLHost replaces the host component of rawURL with newHost, preserving | ||
| // scheme, port (if already present in rawURL), path, and query unchanged. |
There was a problem hiding this comment.
I'm just flagging this, but I don't think we need to change anything here unless there's need for it.
Agreed with
. Assigning u.Host clears any port numbers in the original URL. Copilot suggests we keep it as is and only update the godoc. The problem with that is we'll replace any original port number with the one in the newHost. I think it's almost okay because the original URL is meant to point at GitHub API hosts (e.g. api.github.com) and it's so rare that a production API exposes a non-default 443/80 port publicly.
The other approach, which is to preserve the port can also okay but there's a caveat, which is we should first store the original port number, then replace the host, and if there's no port number, then we replace it. This is because the newHost can be configured with a trailing port number.
| cfg := newRequestConfig(opts) | ||
| clientOpts := clientOptions(hostname, c.http.Transport) | ||
| for name, value := range cfg.headers { | ||
| clientOpts.Headers[name] = value | ||
| } | ||
| if cfg.noFollowRedirects { | ||
| clientOpts.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this piece should be extracted as a function/method, and the reused here and in the GraphQL method.
There was a problem hiding this comment.
Following up on my own comment here: I dug into extracting this and I don't think it's worth it, @williammartin.
The two blocks only really overlap on the header loop. The parts that differ carry subtle contracts: GraphQL sets the feature header before applying caller headers so an explicit WithHeader wins, and the redirect policy is documented as REST only. Any shared helper either flips that header precedence or starts applying redirect handling on the GraphQL path, which is the kind of thing that quietly breaks behaviour when someone touches the shared code later.
Given how small each block is, I'd rather keep them separate than trade a tiny bit of deduplication for that risk. So I'm going to skip this one unless you feel otherwise.
| type httpClient interface { | ||
| Do(*http.Request) (*http.Response, error) | ||
| } |
There was a problem hiding this comment.
Thanks for removing this interface as it was masking HTTP client usages.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
95aa24c to
ecd6795
Compare
|
Pushed a few changes (force-pushed after a rebase onto the latest Align Route attachment uploads through Rebase housekeeping. Three small commits fall out of moving onto the newer
|
gist had no acceptance scripts, so its commands were never exercised against a real host. Cover create, view and delete in one script, and edit, rename and list in another. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
search-issues flakes because gh search reads a separate index that lags issue creation, and five seconds was not reliably enough for it to catch up. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The variable took a single script name, so running a chosen subset meant one go test invocation per script. Accept a comma separated list and select the ones belonging to the command directory under test. A filter that matches nothing in a directory now skips that directory rather than falling back to running all of it, so a mistyped script name reports as a skip instead of silently passing a full run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A call site that builds an absolute api.github.com URL and calls httpClient.Do bypasses any central resolution, and no existing test notices, because api.github.com is reachable from CI. That makes the routing claim unfalsifiable. This harness makes a bypass fail loudly. It runs the real gh binary against a recording TLS reverse proxy, with api.github.com blackholed inside the container, so a request that honours api_host reaches the gateway and a request that ignores it cannot connect at all. Results are recorded by name, and acceptance scripts run one per invocation, so a change that fixes a single call site is visible as that specific assertion turning green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
go-gh gains per-host API endpoint overrides, so a host can route its API traffic through a gateway. That immediately breaks authentication: gh resolves tokens from the hostname in the request URL, and after the override that hostname is the gateway, which gh has never logged in to and holds no token for. Map the gateway back to the host it stands in for and send that host's token. The fallback only applies when the hostname has no token of its own, so a host we do authenticate keeps resolving exactly as before and an api_host mapping cannot hijack real credentials. This is the credential half only. Requests still have to reach the gateway to benefit, and most call sites do not yet. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
gh api builds its request URLs from the hostname directly rather than going through the shared client, so a host's api_host had no effect on it: gh api repos/cli/cli still went to api.github.com even when the host was configured to route elsewhere. Resolve api_host when building the URL for a relative path or graphql. Absolute URLs are deliberately left alone, both because the user asked for that exact URL and because paginating on a rewritten Link header depends on following the gateway's own URLs unchanged. This means api_host is now resolved in two places, which is a smell worth being honest about rather than hiding: gh api takes its path verbatim from the user, so it cannot use the shared client that resolves api_host for everything else. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Being on api.Client is not the same as being routed. RenameRepo already used client.REST, but handed it an absolute URL built from ghinstance.RESTPrefix, so the host was decided before the client saw the request and a configured api_host was ignored. Pass a relative path and let the client resolve the host, as it does for every other call. No new capability is needed here, only the removal of a hardcoded prefix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Many call sites could not move onto api.Client because REST decodes into a receiver, and they need the response itself: a streaming body, a body that is not JSON, a response header, or the status code of a success. Having no way to express that, they built absolute api.github.com URLs and called httpClient.Do, which decides the host before any central resolution can apply. Add Request and RequestWithContext, which return the response for the caller to consume, and migrate those call sites. Non-2xx responses still become an HTTPError, so callers only handle the success path. Add UnexpectedStatusError for callers that require one specific status. Since every non-2xx is already an error, such a caller can only be surprised by a different 2xx, and handing a success to an error parser would be wrong. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Some endpoints do not say which OAuth scopes they require, so gh supplies the answer itself: it calls EndpointNeedsScopes on the response before turning it into an error, which folds the scope into the suggestion the user is shown. That only works while the call site holds the response. A call site that hands request making to the shared client never sees a failed response, because the client has already converted it into an error and closed the body. So the scope has to travel with the request instead. Add WithEndpointScopes, and apply it on the error path: the scope is added to the error's headers before the suggestion is generated, which is the same mechanism as before, moved to where the response still exists. gh gist create is the first caller, and needed this to keep telling a user with an under-scoped token that they are missing the gist scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
gh repo delete is the reason this option exists, and the reason it built its own client rather than sharing one. Deleting a repository that has since been renamed returns a 301, and Go's default redirect policy turns a DELETE into a GET when it follows one. The user would be told the delete succeeded while nothing had been deleted, so the command copied the http.Client, set CheckRedirect on the copy, and made the request itself. Making the request itself is also how it came to name api.github.com and ignore a host's api_host. Add WithoutFollowingRedirects so the policy can be stated per request, and the destination can go back to being the client's business. The option is deliberately REST-only. GraphQL does not meet redirects in practice, and offering it there would suggest a guarantee that is not tested. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
The remaining call sites that build their own request do so for a header, usually an Accept naming a preview media type or a raw representation. Building the request also meant building the URL, which is how these came to name api.github.com and ignore a host's api_host. Add WithHeader, so a call site can say which header it needs without also taking ownership of where the request goes. Headers set this way take precedence over the transport's own, which is what a caller asking for a specific representation means. GraphQL takes options too, for one reason: gh auth login validates a token the client has not been configured with yet, so it must pass an explicit Authorization header. This is the last of the three capabilities, and with it the gateway harness is fully green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
These two call sites are the last outside gh api that send a request through a raw http.Client, and they resisted the shared request surface for a real reason. Uploading an asset sets ContentLength and GetBody so a retry can rewind the file, and neither is expressible as a method, path, body or header. Add DoRequest, which takes a request the caller has built and applies the same error handling as Request. It also sends the request through the client held by api.Client, so a CheckRedirect set on that client survives, which Request cannot promise because go-gh builds a client of its own from the transport. This changes no behaviour. Both sites use absolute URLs the API returned, and those URLs already point wherever the request that produced them went, so routing was never wrong here. What changes is that api.Client is now the single place a request leaves gh, so a later change to how a destination is resolved reaches these two without anyone remembering they exist. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
Note that Go's default redirect policy rewrites any non-GET/HEAD method (not just DELETE) to GET on 301/302/303, and document why repo delete opts out of following redirects to avoid a phantom success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Add Request and RequestWithContext to the SafeURL path construction query so their URL arguments must flow through safeurl. Exclude the client's own internal delegation between these methods, which forwards the caller's already checked path and would otherwise be reported as a hand built URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
…oken Converge cli/cli's origin-comparison policy onto go-gh's host-only policy so both token layers ignore the port when deciding whether a request is same-host. go-gh compares req.URL.Hostname() while cli/cli previously compared the full host including port, making the two layers inconsistent. Rename getHost to getHostname and strip any port from the host so redirect comparison, token lookup, and enterprise detection all key on the hostname alone, matching how gh stores config host keys (auth login rejects a port). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Hold an api.Client on Uploader and post assets with DoRequest instead of calling http.Client.Do directly. DoRequest is built for requests that must set fields Request cannot express, such as ContentLength and GetBody when uploading an asset, and it already turns a non-2xx response into an HTTPError, so the hand-rolled status check and HandleHTTPError call are no longer needed. NewUploader keeps its *http.Client parameter and wraps it, so callers are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
The go-gh per-host api_host branch was rebased onto its latest trunk, so update the pin to its new HEAD. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
2352e08 to
8103f34
Compare
8103f34 to
6865464
Compare
BagToad
left a comment
There was a problem hiding this comment.
I've tested this against a 150 test suite of various invocations of --attach across both github.com and GHEC DR
Relates to #13717
Fixes #13991
Description
There are users out there who would like to configure an
api_hostin theirhosts.yml, such that any requests for the GitHub API are sent to a different host. There were a lot of call sites inghthat made directhttpClient.Dorequests to provided URLs, avoiding any existing api client abstractions that existed incli/cliorcli/go-gh. We've made a lot of progress in the PRs that landed under #13991 with fairly simple lift and shifts. This PR is the Giganotosaurus to cover (nearly) all the remaining places.The only obvious missing piece to me are codespaces commands. I believe they historically actually already have a way to achieve this via setting
GITHUB_API_URLas an env var, but I didn't feel like I was ready to tackle it quite yet.Note this doesn't support setting
api_hostyet, it must be configured in thehosts.ymlmanually.How did you test this change?
docs/api-host-test-harness.mdintroduced in a289d76 introduces a comprehensive test harness (that may never be merged) that runs a series of new tests and a representative subset of our acceptance tests in a container withiptablesset to blackholegithub.com, requiring that API requests go through a local proxy.In each commit after the harness is added, there should be a document that outlines the state of the test harness and acceptance tests at that commit. I red/greened this entire feature by building out the harness first and then addressing each class of issues. It's probably not perfect (see below) but I think it's worked well.
Aside from the subset of tests, I have tested it against the full acceptance suite and found one meaningful bug around telemetry which is in d4f4995 but I didn't include it in this branch because it's not a functional thing to test and just adds noise.
Notes for reviewers
Read the commits one by one from top to bottom, seriously, I put a lot of effort into splitting them into meaningful chunks and I have reviewed most of the content of each to be sure that its not totally incompetent. However, with a chunk of work this size, I wouldn't be surprised if there's some nonsense in there.
One important note is that I tried my best here to just find a non-invasive seam so the interface now exposed by
api.Clientis a little strange (REST,RESTWithNext,Request,RequestWithContext,DoRequest). I do have another branch that goes ham on this problem and creates a whole new abstraction, introducing a totally new client. I think this might be the future, but the risk is high, and I wasn't confident getting it into anyone's hands before vacation.This PR depends on cli/go-gh#275
Authorship and follow-up
Who wrote this:
Who answers review comments: