From bc307e73383c19bb68c1437f670ee9f39605f688 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Fri, 13 Mar 2026 17:53:09 +0000 Subject: [PATCH] merge checker to use the core/httpclient with baseurl transport --- example/server/orchestrator/main.go | 38 ++++++++----------- extension/mergechecker/github/BUILD.bazel | 1 + extension/mergechecker/github/checker.go | 11 ++---- extension/mergechecker/github/checker_test.go | 14 +++---- 4 files changed, 26 insertions(+), 38 deletions(-) diff --git a/example/server/orchestrator/main.go b/example/server/orchestrator/main.go index 5ed6b3ce..e4311d35 100644 --- a/example/server/orchestrator/main.go +++ b/example/server/orchestrator/main.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "net" - "net/http" "os" "os/signal" "sync" @@ -193,7 +192,10 @@ func run() error { c := consumer.New(logger.Sugar(), scope.SubScope("consumer"), registry) // Create merge checker - mc := newMergeChecker(logger, scope) + mc, err := newMergeChecker(logger, scope) + if err != nil { + return fmt.Errorf("failed to create merge checker: %w", err) + } // Create change provider cp, err := newChangeProvider(logger, scope) @@ -547,28 +549,29 @@ func parseTimeout(envVal string, defaultVal time.Duration) time.Duration { } // newMergeChecker creates a MergeChecker for GitHub (github.com). -// Configured via GITHUB_TOKEN and GITHUB_GRAPHQL_URL environment variables. -func newMergeChecker(logger *zap.Logger, scope tally.Scope) mergechecker.MergeChecker { - graphQLURL := os.Getenv("GITHUB_GRAPHQL_URL") - if graphQLURL == "" { - graphQLURL = "https://api.github.com/graphql" +// Configured via GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT environment variables. +func newMergeChecker(logger *zap.Logger, scope tally.Scope) (mergechecker.MergeChecker, error) { + client, err := httpclient.NewClient(getEnv("GITHUB_BASE_URL", "https://api.github.com")) + if err != nil { + return nil, fmt.Errorf("failed to build GitHub HTTP client: %w", err) } - httpClient := &http.Client{} if token := os.Getenv("GITHUB_TOKEN"); token != "" { - httpClient.Transport = &bearerTransport{token: token} + ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) + client.Transport = &oauth2.Transport{Source: ts, Base: client.Transport} } + client.Timeout = parseTimeout(os.Getenv("GITHUB_TIMEOUT"), 30*time.Second) + github := githubchecker.NewMergeChecker(githubchecker.Params{ - HTTPClient: httpClient, - GraphQLURL: graphQLURL, + HTTPClient: client, Logger: logger.Sugar(), MetricsScope: scope.SubScope("mergechecker"), }) return mergechecker.NewMultiChecker(map[string]mergechecker.MergeChecker{ "github": github, - }) + }), nil } // newChangeProvider creates a ChangeProvider for GitHub (github.com). @@ -592,14 +595,3 @@ func newChangeProvider(logger *zap.Logger, scope tally.Scope) (changeprovider.Ch MetricsScope: scope.SubScope("changeprovider"), }), nil } - -// bearerTransport is an http.RoundTripper that adds a Bearer token to requests. -type bearerTransport struct { - token string -} - -func (t *bearerTransport) RoundTrip(req *http.Request) (*http.Response, error) { - req = req.Clone(req.Context()) - req.Header.Set("Authorization", "Bearer "+t.token) - return http.DefaultTransport.RoundTrip(req) -} diff --git a/extension/mergechecker/github/BUILD.bazel b/extension/mergechecker/github/BUILD.bazel index df212f17..ff1a2cba 100644 --- a/extension/mergechecker/github/BUILD.bazel +++ b/extension/mergechecker/github/BUILD.bazel @@ -27,6 +27,7 @@ go_test( ], embed = [":github"], deps = [ + "//core/httpclient", "//entity", "//entity/github", "//extension/mergechecker", diff --git a/extension/mergechecker/github/checker.go b/extension/mergechecker/github/checker.go index a3d26191..41fed73c 100644 --- a/extension/mergechecker/github/checker.go +++ b/extension/mergechecker/github/checker.go @@ -31,12 +31,9 @@ import ( // Params holds the dependencies for the GitHub MergeChecker. type Params struct { - // HTTPClient is a pre-configured HTTP client with auth (bearer token, GitHub App JWT, etc.). - // Auth is the caller's responsibility via HTTP transport/round-tripper. + // HTTPClient is a pre-configured HTTP client. The caller is responsible for + // configuring the base URL (via BaseURLTransport) and auth (via a transport layer). HTTPClient *http.Client - // GraphQLURL is the GitHub GraphQL API endpoint - // (e.g., "https://api.github.com/graphql" or "https://ghe.example.com/api/graphql"). - GraphQLURL string // Logger is the structured logger. Logger *zap.SugaredLogger // MetricsScope is the metrics scope for instrumentation. @@ -46,7 +43,6 @@ type Params struct { // mergeChecker implements the mergechecker.MergeChecker interface using the GitHub GraphQL API. type mergeChecker struct { httpClient *http.Client - graphQLURL string logger *zap.SugaredLogger metricsScope tally.Scope } @@ -58,7 +54,6 @@ var _ mergechecker.MergeChecker = (*mergeChecker)(nil) func NewMergeChecker(params Params) mergechecker.MergeChecker { return &mergeChecker{ httpClient: params.HTTPClient, - graphQLURL: params.GraphQLURL, logger: params.Logger.Named("github_mergechecker"), metricsScope: params.MetricsScope.SubScope("github_mergechecker"), } @@ -120,7 +115,7 @@ func (c *mergeChecker) fetchPRInfo(ctx context.Context, changeIDs []entitygithub return nil, fmt.Errorf("failed to marshal graphql request: %w", err) } - httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, c.graphQLURL, bytes.NewReader(reqBody)) + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, "/graphql", bytes.NewReader(reqBody)) if err != nil { return nil, fmt.Errorf("failed to create http request: %w", err) } diff --git a/extension/mergechecker/github/checker_test.go b/extension/mergechecker/github/checker_test.go index 7cd3d4d9..e39f36c2 100644 --- a/extension/mergechecker/github/checker_test.go +++ b/extension/mergechecker/github/checker_test.go @@ -25,20 +25,20 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally/v4" + "github.com/uber/submitqueue/core/httpclient" "github.com/uber/submitqueue/entity" "github.com/uber/submitqueue/extension/mergechecker" "go.uber.org/zap/zaptest" ) func newTestMergeChecker(t *testing.T, serverURL string) mergechecker.MergeChecker { - logger := zaptest.NewLogger(t).Sugar() - scope := tally.NoopScope - + t.Helper() + client, err := httpclient.NewClient(serverURL) + require.NoError(t, err) return NewMergeChecker(Params{ - HTTPClient: &http.Client{}, - GraphQLURL: serverURL, - Logger: logger, - MetricsScope: scope, + HTTPClient: client, + Logger: zaptest.NewLogger(t).Sugar(), + MetricsScope: tally.NoopScope, }) }