You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
config.go defines GithubEnterprise, httpclient.go defines GetGHE and GHEKey and GHEKeysToKeys. None are reachable from cmd.go:Run's code path — the YAML config only deserializes Sources (slice of Source, which is just {URL string}), and only client.GetURL is ever called. The GHE types and methods are dead code.
Option A is the lower-effort path and is the right call if the maintainer doesn't have a near-term plan for GHE adoption. Option B is right if there's a concrete user / use-case waiting on it.
A third option — leave it as-is — has a real ongoing cost (the maintenance + #4 ambiguity above) so I'd avoid it.
Note on history
The pre-v1.0.0 commit a81fede "remove mostly-unimplemented support for GHE" already attempted a partial removal. The struct definition and the methods survived that pass. Worth understanding what that commit intentionally kept (if anything) before removing the rest.
Acceptance criteria (option A)
No remaining reference to GithubEnterprise, GHEKey, GetGHE, GHEKeysToKeys in any non-test file.
encoding/json import is removed from httpclient.go if nothing else needs it.
Summary
config.godefinesGithubEnterprise,httpclient.godefinesGetGHEandGHEKeyandGHEKeysToKeys. None are reachable fromcmd.go:Run's code path — the YAML config only deserializesSources(slice ofSource, which is just{URL string}), and onlyclient.GetURLis ever called. The GHE types and methods are dead code.Why this matters
Three concrete costs of leaving it in:
Latent security risk — issue httpclient.go: cache key is the URL only, so different auth tokens for the same URL share cached data #4 is specifically about a cache-key bug in
GetGHE(Authorization header isn't part of the cache key). That issue is rated Medium because it's "latent until GHE is wired up". If GHE is never going to be wired up, httpclient.go: cache key is the URL only, so different auth tokens for the same URL share cached data #4's risk is permanent (because the latent code keeps existing in the binary), and if GHE is going to be wired up, httpclient.go: cache key is the URL only, so different auth tokens for the same URL share cached data #4 becomes Critical the day that lands. Either way, the dead code makes the security posture less clear.Maintenance load on every PR — every time someone touches
httpclient.go(PR Stop killing the process on HTTP fetch errors (closes #2) #10 did, this issue's neighbor PRs likely will), they have to read and reason about the GHE branch, even though it's unreachable. PR Stop killing the process on HTTP fetch errors (closes #2) #10 specifically had to make the samelog.Fatal-removal changes inGetGHEthat it made inGetURL, doubling the diff for no runtime benefit.Misleading code surface — a maintainer reading the code reasonably believes ussher supports GitHub Enterprise. It doesn't, today.
Approach — two options, pick one
A. Remove the dead code
GithubEnterprisefromconfig.go.GHEKey,GHEKeysToKeys,GetGHEfromhttpclient.go.encoding/jsonimport (currently only used by the GHE path).B. Wire it up
github_enterprisefield (or list of GHE entries) toConfig.LoadConfigByPathso YAML deserializes them.cmd.go:Run, iterate over the GHE entries alongsideSourcesand dispatch toGetGHE.Option A is the lower-effort path and is the right call if the maintainer doesn't have a near-term plan for GHE adoption. Option B is right if there's a concrete user / use-case waiting on it.
A third option — leave it as-is — has a real ongoing cost (the maintenance + #4 ambiguity above) so I'd avoid it.
Note on history
The pre-v1.0.0 commit
a81fede "remove mostly-unimplemented support for GHE"already attempted a partial removal. The struct definition and the methods survived that pass. Worth understanding what that commit intentionally kept (if anything) before removing the rest.Acceptance criteria (option A)
GithubEnterprise,GHEKey,GetGHE,GHEKeysToKeysin any non-test file.encoding/jsonimport is removed fromhttpclient.goif nothing else needs it.TestGetGHE_UnreachableIsNotFatalis removed (and TestGetGHE_UnreachableIsNotFatal performs a real DNS lookup; flaky in offline CI #21 is closed by removal).Acceptance criteria (option B)
api_hostname,user,token.RuninvokesGetGHEfor each declared GHE source.Files
/home/user/ussher/config.go/home/user/ussher/httpclient.go/home/user/ussher/httpclient_test.go/home/user/ussher/cmd.go,/home/user/ussher/README.md