Uh oh!
There was an error while loading. Please reload this page.
[pull] trunk from cli:trunk - #327
Merged
Merged
Conversation
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
HostForAPIHost iterated c.Hosts() in undefined order, so when several hosts share one api_host the winner was unstable. Sort a clone of the host list first to make the first-match resolution deterministic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Replace errors.As plus a separate variable declaration with the generic errors.AsType across the call sites introduced by the api client rollout. 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
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
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
Improve automated issue triage analysis
Honour api_host across the CLI, and give api.Client a flexible request surface
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )