Skip to content

Commit 1642331

Browse files
Refactor GitHub provider to use Config pattern
Replace Params with Config to encapsulate BaseURL + Token + Timeout together, addressing review feedback about HTTPClient encapsulation. Changes: - Add Config type with BaseURL, Token, Timeout, and optional HTTPClient - Derive GraphQL URL from BaseURL (no separate parameter needed) - Add configurable timeout (defaults to 30s) - Simplify main.go to use DefaultConfig() - Update all tests to use Config instead of Params Benefits: - Impossible to misconfigure (URL and auth bundled) - Multi-instance support (github.com + GHE) - Flexible timeout per instance - Cleaner API surface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent a3b5ed8 commit 1642331

5 files changed

Lines changed: 171 additions & 132 deletions

File tree

example/server/orchestrator/main.go

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -448,25 +448,10 @@ func newMergeChecker(logger *zap.Logger, scope tally.Scope) mergechecker.MergeCh
448448
}
449449

450450
// newChangeProvider creates a ChangeProvider for GitHub (github.com).
451-
// Configured via GITHUB_TOKEN and GITHUB_GRAPHQL_URL environment variables.
452-
// Reuses the same HTTP client configuration as the mergechecker.
451+
// Configured via GITHUB_BASE_URL and GITHUB_TOKEN environment variables.
453452
func newChangeProvider(logger *zap.Logger, scope tally.Scope) changeprovider.ChangeProvider {
454-
graphQLURL := os.Getenv("GITHUB_GRAPHQL_URL")
455-
if graphQLURL == "" {
456-
graphQLURL = "https://api.github.com/graphql"
457-
}
458-
459-
httpClient := &http.Client{}
460-
if token := os.Getenv("GITHUB_TOKEN"); token != "" {
461-
httpClient.Transport = &bearerTransport{token: token}
462-
}
463-
464-
return githubprovider.NewProvider(githubprovider.Params{
465-
HTTPClient: httpClient,
466-
GraphQLURL: graphQLURL,
467-
Logger: logger.Sugar(),
468-
MetricsScope: scope.SubScope("changeprovider"),
469-
})
453+
config := githubprovider.DefaultConfig()
454+
return githubprovider.NewProvider(config, logger.Sugar(), scope.SubScope("changeprovider"))
470455
}
471456

472457
// bearerTransport is an http.RoundTripper that adds a Bearer token to requests.

extension/changeprovider/github/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")
33
go_library(
44
name = "github",
55
srcs = [
6+
"config.go",
67
"convert.go",
78
"graphql.go",
89
"provider.go",
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package github
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"os"
7+
"time"
8+
)
9+
10+
const (
11+
// DefaultTimeout is the default HTTP request timeout for GitHub API calls.
12+
// This applies to the entire request/response cycle.
13+
DefaultTimeout = 30 * time.Second
14+
)
15+
16+
// Config holds configuration for connecting to a GitHub backend.
17+
type Config struct {
18+
// BaseURL is the GitHub instance base URL (without /graphql suffix).
19+
// Examples: "https://api.github.com", "https://ghe.company.com"
20+
BaseURL string
21+
22+
// Token for authenticating to this GitHub instance.
23+
// Can be empty for unauthenticated requests.
24+
Token string
25+
26+
// Timeout for HTTP requests to this GitHub instance.
27+
// If zero or negative, defaults to DefaultTimeout (30s).
28+
// Set this higher for slow GHE instances or flaky networks.
29+
Timeout time.Duration
30+
31+
// HTTPClient provides complete control over the HTTP client.
32+
// If set, BaseURL is still used but Token and Timeout are ignored.
33+
// Use this for custom transports, connection pooling, or testing.
34+
HTTPClient *http.Client
35+
}
36+
37+
// Validate checks if the config is valid.
38+
func (c Config) Validate() error {
39+
if c.BaseURL == "" {
40+
return fmt.Errorf("BaseURL is required")
41+
}
42+
return nil
43+
}
44+
45+
// DefaultConfig returns a Config for github.com from environment.
46+
func DefaultConfig() Config {
47+
return Config{
48+
BaseURL: getEnvOrDefault("GITHUB_BASE_URL", "https://api.github.com"),
49+
Token: os.Getenv("GITHUB_TOKEN"),
50+
Timeout: 0, // Will use DefaultTimeout
51+
}
52+
}
53+
54+
func getEnvOrDefault(key, defaultVal string) string {
55+
if val := os.Getenv(key); val != "" {
56+
return val
57+
}
58+
return defaultVal
59+
}

extension/changeprovider/github/provider.go

Lines changed: 59 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,6 @@ import (
1515
"github.com/uber/submitqueue/extension/changeprovider"
1616
)
1717

18-
// Params holds the dependencies for the GitHub ChangeProvider.
19-
type Params struct {
20-
// HTTPClient is a pre-configured HTTP client with auth (bearer token, GitHub App JWT, etc.).
21-
// Auth is the caller's responsibility via HTTP transport/round-tripper.
22-
HTTPClient *http.Client
23-
// GraphQLURL is the GitHub GraphQL API endpoint
24-
// (e.g., "https://api.github.com/graphql" or "https://ghe.example.com/api/graphql").
25-
GraphQLURL string
26-
// Logger is the structured logger.
27-
Logger *zap.SugaredLogger
28-
// MetricsScope is the metrics scope for instrumentation.
29-
MetricsScope tally.Scope
30-
}
31-
3218
// provider implements the ChangeProvider interface for GitHub.
3319
type provider struct {
3420
httpClient *http.Client
@@ -37,14 +23,67 @@ type provider struct {
3723
metrics tally.Scope
3824
}
3925

40-
// NewProvider creates a new GitHub ChangeProvider.
41-
func NewProvider(params Params) changeprovider.ChangeProvider {
26+
// NewProvider creates a new GitHub ChangeProvider from configuration.
27+
func NewProvider(config Config, logger *zap.SugaredLogger, metrics tally.Scope) changeprovider.ChangeProvider {
28+
if err := config.Validate(); err != nil {
29+
panic(fmt.Sprintf("invalid GitHub config: %v", err))
30+
}
31+
32+
// Derive GraphQL URL from base URL
33+
graphQLURL := config.BaseURL + "/graphql"
34+
35+
// Use provided client or create default
36+
httpClient := config.HTTPClient
37+
if httpClient == nil {
38+
timeout := config.Timeout
39+
if timeout <= 0 {
40+
timeout = DefaultTimeout
41+
}
42+
httpClient = createDefaultClient(config.Token, timeout)
43+
}
44+
4245
return &provider{
43-
httpClient: params.HTTPClient,
44-
graphQLURL: params.GraphQLURL,
45-
logger: params.Logger.Named("github_changeprovider"),
46-
metrics: params.MetricsScope.SubScope("github_changeprovider"),
46+
httpClient: httpClient,
47+
graphQLURL: graphQLURL,
48+
logger: logger.Named("github_changeprovider"),
49+
metrics: metrics.SubScope("github_changeprovider"),
50+
}
51+
}
52+
53+
// createDefaultClient creates an HTTP client with the given token and timeout.
54+
func createDefaultClient(token string, timeout time.Duration) *http.Client {
55+
transport := createTransport(token)
56+
57+
return &http.Client{
58+
Timeout: timeout,
59+
Transport: transport,
60+
}
61+
}
62+
63+
// createTransport creates an HTTP transport with optional bearer token authentication.
64+
func createTransport(token string) http.RoundTripper {
65+
base := http.DefaultTransport
66+
67+
if token == "" {
68+
return base
4769
}
70+
71+
return &bearerTransport{
72+
token: token,
73+
base: base,
74+
}
75+
}
76+
77+
// bearerTransport is an http.RoundTripper that adds a Bearer token to requests.
78+
type bearerTransport struct {
79+
token string
80+
base http.RoundTripper
81+
}
82+
83+
func (t *bearerTransport) RoundTrip(req *http.Request) (*http.Response, error) {
84+
req = req.Clone(req.Context())
85+
req.Header.Set("Authorization", "Bearer "+t.token)
86+
return t.base.RoundTrip(req)
4887
}
4988

5089
// Get retrieves change information from GitHub for the provided Change.

0 commit comments

Comments
 (0)