make oauth and timeout setup happen outside the http client

Author: rashmi-prithyani

core/httpclient/BUILD.bazel

@@ -5,7 +5,6 @@ go_library(
     srcs = ["transport.go"],
     importpath = "github.com/uber/submitqueue/core/httpclient",
     visibility = ["//visibility:public"],
-    deps = ["@org_golang_x_oauth2//:oauth2"],
 )
 
 go_test(

core/httpclient/transport.go

@@ -4,9 +4,6 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
-	"time"
-
-	"golang.org/x/oauth2"
 )
 
 // BaseURLTransport is an http.RoundTripper that rewrites every request URL
@@ -39,24 +36,17 @@ func (t *BaseURLTransport) RoundTrip(req *http.Request) (*http.Response, error)
 	return next.RoundTrip(newReq)
 }
 
-// NewClient builds an *http.Client with BaseURLTransport and optionally
-// oauth2 bearer auth configured. The transport chain is:
-//
-//	oauth2.Transport (if token provided) → BaseURLTransport → DefaultTransport
-func NewClient(rawBaseURL, token string, timeout time.Duration) (*http.Client, error) {
+// NewClient builds an *http.Client with BaseURLTransport configured.
+// Callers are responsible for layering additional transports (e.g. auth) and
+// setting Timeout on the returned client.
+func NewClient(rawBaseURL string) (*http.Client, error) {
 	u, err := url.Parse(rawBaseURL)
 	if err != nil {
 		return nil, err
 	}
 
-	var transport http.RoundTripper = &BaseURLTransport{
+	return &http.Client{Transport: &BaseURLTransport{
 		BaseURL: u,
 		Next:    http.DefaultTransport,
-	}
-	if token != "" {
-		ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
-		transport = &oauth2.Transport{Source: ts, Base: transport}
-	}
-
-	return &http.Client{Transport: transport, Timeout: timeout}, nil
+	}}, nil
 }

core/httpclient/transport_test.go

@@ -2,10 +2,8 @@ package httpclient
 
 import (
 	"net/http"
-	"net/http/httptest"
 	"net/url"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -78,55 +76,10 @@ func TestBaseURLTransport_DoesNotMutateOriginalRequest(t *testing.T) {
 }
 
 func TestNewClient_InvalidURL(t *testing.T) {
-	_, err := NewClient("://invalid", "", 30*time.Second)
+	_, err := NewClient("://invalid")
 	require.Error(t, err)
 }
 
-func TestNewClient_SetsTimeout(t *testing.T) {
-	client, err := NewClient("https://api.github.com", "", 10*time.Second)
-	require.NoError(t, err)
-	assert.Equal(t, 10*time.Second, client.Timeout)
-}
-
-func TestNewClient_AuthHeader(t *testing.T) {
-	tests := []struct {
-		name           string
-		token          string
-		wantAuthHeader string
-	}{
-		{
-			name:           "no token, no auth header",
-			token:          "",
-			wantAuthHeader: "",
-		},
-		{
-			name:           "with token, adds bearer auth header",
-			token:          "my-token",
-			wantAuthHeader: "Bearer my-token",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				w.Header().Set("X-Captured-Auth", r.Header.Get("Authorization"))
-				w.WriteHeader(http.StatusOK)
-			}))
-			defer server.Close()
-
-			client, err := NewClient(server.URL, tt.token, 30*time.Second)
-			require.NoError(t, err)
-
-			req, err := http.NewRequest(http.MethodGet, "/", nil)
-			require.NoError(t, err)
-
-			resp, err := client.Do(req)
-			require.NoError(t, err)
-			assert.Equal(t, tt.wantAuthHeader, resp.Header.Get("X-Captured-Auth"))
-		})
-	}
-}
-
 func mustParseURL(t *testing.T, raw string) *url.URL {
 	t.Helper()
 	u, err := url.Parse(raw)

example/server/orchestrator/BUILD.bazel

@@ -41,6 +41,7 @@ go_library(
         "@com_github_uber_go_tally_v4//:tally",
         "@org_golang_google_grpc//:grpc",
         "@org_golang_google_grpc//reflection",
+        "@org_golang_x_oauth2//:oauth2",
         "@org_uber_go_zap//:zap",
     ],
 )

example/server/orchestrator/main.go

@@ -28,6 +28,8 @@ import (
 	"time"
 
 	_ "github.com/go-sql-driver/mysql"
+	"golang.org/x/oauth2"
+
 	"github.com/uber-go/tally/v4"
 	"github.com/uber/submitqueue/core/consumer"
 	"github.com/uber/submitqueue/core/httpclient"
@@ -572,15 +574,18 @@ func newMergeChecker(logger *zap.Logger, scope tally.Scope) mergechecker.MergeCh
 // newChangeProvider creates a ChangeProvider for GitHub (github.com).
 // Configured via GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT environment variables.
 func newChangeProvider(logger *zap.Logger, scope tally.Scope) (changeprovider.ChangeProvider, error) {
-	client, err := httpclient.NewClient(
-		getEnv("GITHUB_BASE_URL", "https://api.github.com"),
-		os.Getenv("GITHUB_TOKEN"),
-		parseTimeout(os.Getenv("GITHUB_TIMEOUT"), 30*time.Second),
-	)
+	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)
 	}
 
+	if token := os.Getenv("GITHUB_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)
+
 	return githubprovider.NewProvider(githubprovider.Params{
 		HTTPClient:   client,
 		Logger:       logger.Sugar(),

extension/changeprovider/github/provider_test.go

@@ -6,7 +6,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -20,7 +19,7 @@ import (
 
 func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvider {
 	t.Helper()
-	client, err := httpclient.NewClient(serverURL, "", 30*time.Second)
+	client, err := httpclient.NewClient(serverURL)
 	require.NoError(t, err)
 	return NewProvider(Params{
 		HTTPClient:   client,