Uh oh!
There was an error while loading. Please reload this page.
Disable telemetry for unauthenticated GHES gh api requests using absolute hostnames - #14337
Conversation
gh api requestsThere was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The exported HTTP client injection contract must be preserved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
pkg/cmd/api/api.go — 🛑 Requirement: Preserve the exported HTTP client injection contract. Changing this field from a… |
What changed in this PR
Routes telemetry suppression into gh api’s specialized HTTP client for GHES requests.
Changes:
- Propagates the telemetry disabler through command factories.
- Configures API clients to suppress GHES telemetry.
- Adds GHES and GitHub.com regression tests.
Blocking issue: 🛑 Critical (2 votes) — Preserve the exported HttpClient injection signature in ApiOptions; changing it breaks existing consumers. Capture TelemetryDisabler in the default closure or use a separate unexported factory.
| File | Description |
|---|---|
pkg/cmdutil/factory.go | Exposes the telemetry disabler. |
pkg/cmd/factory/default.go | Initializes the new factory field. |
pkg/cmd/api/api.go | Passes telemetry configuration into the API client. |
pkg/cmd/api/api_test.go | Tests hostname and absolute-URL telemetry behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
gh api requestsgh api requests using absolute hostnamesCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
6b30ab7 to
4a75987Compare
Description
gh apibuilds a specialized HTTP client to support command-specific cache and verbose behavior. This API Client was not provided with aTelemetryDisablerat construction time. It was therefore possible forgh api --hostnameorgh api <absolute url>to emit telemetry unexpectedly under extremely narrow conditions:mightBeGHESUserdisables telemetry:cli/internal/ghcmd/cmd.go
Lines 504 to 506 in 6e82bc5
mightBeGHESUserdisables telemetry:cli/internal/ghcmd/cmd.go
Lines 495 to 497 in 6e82bc5
Under these circumstances, an unauthenticated request using
gh apicould send telemetry if sampled at a 1% chance:How did you test this change?
Before
See above
After
Authorship and follow-up
Who wrote this:
Who answers review comments: