fixup! move some more variables into the httpClient

Author: JamyDev

submitqueue/extension/buildrunner/buildkite/BUILD.bazel

@@ -6,7 +6,6 @@ go_library(
         "buildkite.go",
         "client.go",
         "config.go",
-        "factory.go",
     ],
     importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/buildkite",
     visibility = ["//visibility:public"],
@@ -21,6 +20,7 @@ go_test(
     srcs = ["buildkite_test.go"],
     embed = [":buildkite"],
     deps = [
+        "//core/httpclient",
         "//submitqueue/entity",
         "//submitqueue/extension/buildrunner",
         "@com_github_stretchr_testify//assert",

submitqueue/extension/buildrunner/buildkite/buildkite.go

@@ -46,7 +46,6 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
-	"strings"
 	"sync"
 	"time"
 
@@ -146,19 +145,11 @@ type Params struct {
 // pipeline and starts its background worker goroutine. The goroutine runs for
 // the lifetime of the process.
 //
-// Returns an error if OrgSlug, PipelineSlug, or Branch are empty.
+// The HTTPClient must have BaseURLTransport configured to the pipeline's API
+// root (e.g. "https://api.buildkite.com/v2/organizations/{org}/pipelines/{slug}"),
+// and an auth transport that injects the Authorization header.
 func NewBuildRunner(params Params) (buildrunner.BuildRunner, error) {
 	cfg := params.Config
-	if cfg.OrgSlug == "" {
-		return nil, fmt.Errorf("buildkite: OrgSlug is required")
-	}
-	if cfg.PipelineSlug == "" {
-		return nil, fmt.Errorf("buildkite: PipelineSlug is required")
-	}
-	if cfg.Branch == "" {
-		return nil, fmt.Errorf("buildkite: Branch is required")
-	}
-
 	httpClient := params.HTTPClient
 	if httpClient == nil {
 		httpClient = http.DefaultClient
@@ -231,11 +222,11 @@ func (r *runner) Status(ctx context.Context, buildID entity.BuildID) (entity.Bui
 	var resp buildResponse
 	switch {
 	case cached:
-		org, pipeline, number, err := parseBuildRef(ref)
+		number, err := parseBuildRef(ref)
 		if err != nil {
 			return entity.BuildStatusUnknown, nil, fmt.Errorf("buildkite: malformed build ref: %w", err)
 		}
-		resp, err = r.client.getBuild(ctx, org, pipeline, number)
+		resp, err = r.client.getBuild(ctx, number)
 		if err != nil {
 			return entity.BuildStatusUnknown, nil, fmt.Errorf("buildkite: get build: %w", err)
 		}
@@ -246,7 +237,7 @@ func (r *runner) Status(ctx context.Context, buildID entity.BuildID) (entity.Bui
 			// loop in Accepted forever.
 			return entity.BuildStatusFailed, entity.BuildMetadata{"error": reason}, nil
 		}
-		found, exists, err := r.client.findBuildByMeta(ctx, r.cfg.OrgSlug, r.cfg.PipelineSlug, metaKeyBuildID, buildID.ID)
+		found, exists, err := r.client.findBuildByMeta(ctx, metaKeyBuildID, buildID.ID)
 		if err != nil {
 			return entity.BuildStatusUnknown, nil, fmt.Errorf("buildkite: find build: %w", err)
 		}
@@ -256,7 +247,7 @@ func (r *runner) Status(ctx context.Context, buildID entity.BuildID) (entity.Bui
 			// loop keeps waiting without treating this as terminal.
 			return entity.BuildStatusAccepted, nil, nil
 		}
-		ref = encodeBuildRef(r.cfg.OrgSlug, r.cfg.PipelineSlug, found.Number)
+		ref = encodeBuildRef(found.Number)
 		r.storeRef(buildID.ID, ref)
 		resp = found
 	}
@@ -305,7 +296,6 @@ func (r *runner) processTrigger(job triggerJob) {
 	headJSON, _ := json.Marshal(job.headURIs)
 
 	req := createBuildRequest{
-		Branch:  r.cfg.Branch,
 		Message: "submitqueue speculative build",
 		Env: map[string]string{
 			EnvKeyBaseURIs: string(baseJSON),
@@ -322,15 +312,15 @@ func (r *runner) processTrigger(job triggerJob) {
 		ctx, cancel := r.opCtx()
 		defer cancel()
 		var e error
-		resp, e = r.client.createBuild(ctx, r.cfg.OrgSlug, r.cfg.PipelineSlug, req)
+		resp, e = r.client.createBuild(ctx, req)
 		return e
 	})
 	if err != nil {
 		r.markSubmitFailed(job.buildID, fmt.Sprintf("buildkite submission failed after retries: %v", err))
 		return
 	}
 
-	r.storeRef(job.buildID, encodeBuildRef(r.cfg.OrgSlug, r.cfg.PipelineSlug, resp.Number))
+	r.storeRef(job.buildID, encodeBuildRef(resp.Number))
 }
 
 // processCancel cancels the Buildkite build, recovering its reference from the
@@ -340,26 +330,26 @@ func (r *runner) processCancel(job cancelJob) {
 	ref, cached := r.lookupRef(job.buildID)
 	if !cached {
 		ctx, cancel := r.opCtx()
-		found, exists, err := r.client.findBuildByMeta(ctx, r.cfg.OrgSlug, r.cfg.PipelineSlug, metaKeyBuildID, job.buildID)
+		found, exists, err := r.client.findBuildByMeta(ctx, metaKeyBuildID, job.buildID)
 		cancel()
 		if err != nil || !exists {
 			// Nothing to cancel (not yet submitted) or a transient lookup
 			// failure; the caller may re-issue Cancel.
 			return
 		}
-		ref = encodeBuildRef(r.cfg.OrgSlug, r.cfg.PipelineSlug, found.Number)
+		ref = encodeBuildRef(found.Number)
 		r.storeRef(job.buildID, ref)
 	}
 
-	org, pipeline, number, err := parseBuildRef(ref)
+	number, err := parseBuildRef(ref)
 	if err != nil {
 		return
 	}
 
 	_ = r.withRetry(func() error {
 		ctx, cancel := r.opCtx()
 		defer cancel()
-		return r.client.cancelBuild(ctx, org, pipeline, number)
+		return r.client.cancelBuild(ctx, number)
 	})
 }
 
@@ -450,30 +440,19 @@ func flattenURIs(changes []entity.Change) []string {
 	return uris
 }
 
-// encodeBuildRef encodes org, pipeline, and build number into the internal
-// reference string stored in r.refs.
-// Format: "{org}/{pipeline}/{number}". Buildkite slugs are [a-z0-9-] so "/"
-// is unambiguous as a separator.
-func encodeBuildRef(org, pipeline string, number int) string {
-	return fmt.Sprintf("%s/%s/%d", org, pipeline, number)
+// encodeBuildRef encodes a Buildkite build number as the internal reference
+// string stored in r.refs.
+func encodeBuildRef(number int) string {
+	return strconv.Itoa(number)
 }
 
 // parseBuildRef is the inverse of encodeBuildRef.
-func parseBuildRef(ref string) (org, pipeline string, number int, err error) {
-	last := strings.LastIndex(ref, "/")
-	if last < 1 {
-		return "", "", 0, fmt.Errorf("invalid build ref %q", ref)
-	}
-	number, err = strconv.Atoi(ref[last+1:])
+func parseBuildRef(ref string) (int, error) {
+	n, err := strconv.Atoi(ref)
 	if err != nil {
-		return "", "", 0, fmt.Errorf("invalid build ref %q: non-numeric number segment", ref)
-	}
-	prefix := ref[:last]
-	first := strings.Index(prefix, "/")
-	if first < 1 {
-		return "", "", 0, fmt.Errorf("invalid build ref %q", ref)
+		return 0, fmt.Errorf("invalid build ref %q", ref)
 	}
-	return prefix[:first], prefix[first+1:], number, nil
+	return n, nil
 }
 
 // mapState maps a Buildkite build state string to a BuildStatus.

submitqueue/extension/buildrunner/buildkite/buildkite_test.go

@@ -42,10 +42,7 @@ func newTestRunner(t *testing.T, handler http.Handler) *runner {
 	require.NoError(t, err)
 	return newRunner(
 		Config{
-			OrgSlug:           "test-org",
-			PipelineSlug:      "my-pipeline",
 			QueueName:         "my-queue",
-			Branch:            "main",
 			SubmitTimeout:     5 * time.Second,
 			MaxSubmitAttempts: 3,
 			SubmitBackoff:     time.Millisecond,
@@ -100,32 +97,11 @@ func emptyListHandler(t *testing.T) http.HandlerFunc {
 // --- Interface / constructor ---
 
 func TestNew_ImplementsInterface(t *testing.T) {
-	r, err := NewBuildRunner(Params{Config: Config{
-		OrgSlug:      "org",
-		PipelineSlug: "pipeline",
-		Branch:       "main",
-	}})
+	r, err := NewBuildRunner(Params{})
 	require.NoError(t, err)
 	var _ buildrunner.BuildRunner = r
 }
 
-func TestNew_Validation(t *testing.T) {
-	tests := []struct {
-		name   string
-		params Params
-	}{
-		{"missing org", Params{Config: Config{PipelineSlug: "p", Branch: "main"}}},
-		{"missing pipeline", Params{Config: Config{OrgSlug: "org", Branch: "main"}}},
-		{"missing branch", Params{Config: Config{OrgSlug: "org", PipelineSlug: "p"}}},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			_, err := NewBuildRunner(tt.params)
-			require.Error(t, err)
-		})
-	}
-}
-
 // --- Trigger ---
 
 func TestTrigger_EnqueuesJobAndReturnsID(t *testing.T) {
@@ -177,7 +153,6 @@ func TestTrigger_SubmitsCorrectPayloadToBuildkite(t *testing.T) {
 
 	var req createBuildRequest
 	require.NoError(t, json.Unmarshal(capturedBody, &req))
-	assert.Equal(t, "main", req.Branch)
 	assert.Equal(t, `["github://org/repo/pull/1/aaa111"]`, req.Env[EnvKeyBaseURIs])
 	assert.Equal(t, `["github://org/repo/pull/2/bbb222"]`, req.Env[EnvKeyHeadURIs])
 	assert.Equal(t, "my-queue", req.Env[EnvKeyQueue])
@@ -188,7 +163,7 @@ func TestTrigger_SubmitsCorrectPayloadToBuildkite(t *testing.T) {
 	// After a successful submit the ref is cached, so Status uses getBuild.
 	ref, ok := r.lookupRef(id.ID)
 	require.True(t, ok)
-	assert.Equal(t, encodeBuildRef("test-org", "my-pipeline", 42), ref)
+	assert.Equal(t, encodeBuildRef(42), ref)
 }
 
 func TestTrigger_EmptyBase_ProducesJSONArray(t *testing.T) {
@@ -235,7 +210,7 @@ func TestTrigger_MultipleChangesFlattened(t *testing.T) {
 
 func TestTrigger_QueueFull_ReturnsError(t *testing.T) {
 	r := newRunner(
-		Config{OrgSlug: "org", PipelineSlug: "p", Branch: "main"},
+		Config{},
 		&client{httpClient: http.DefaultClient},
 		1, 1,
 	)
@@ -267,7 +242,7 @@ func TestProcessTrigger_RetriesTransientFailureThenSucceeds(t *testing.T) {
 	assert.Equal(t, 2, posts, "submit should retry after a transient failure")
 	ref, ok := r.lookupRef(id.ID)
 	require.True(t, ok)
-	assert.Equal(t, encodeBuildRef("test-org", "my-pipeline", 7), ref)
+	assert.Equal(t, encodeBuildRef(7), ref)
 }
 
 func TestTrigger_SubmitExhaustsRetries_BuildFails(t *testing.T) {
@@ -331,7 +306,7 @@ func TestStatus_ReturnsLiveBuildkiteState(t *testing.T) {
 	}))
 
 	// Inject ref directly (simulates successful processTrigger).
-	r.storeRef("some-id", encodeBuildRef("test-org", "my-pipeline", 7))
+	r.storeRef("some-id", encodeBuildRef(7))
 
 	status, meta, err := r.Status(context.Background(), entity.BuildID{ID: "some-id"})
 	require.NoError(t, err)
@@ -360,14 +335,14 @@ func TestStatus_RecoversRefAfterCacheMiss(t *testing.T) {
 	// The recovered ref is now cached for subsequent calls.
 	ref, ok := r.lookupRef("bk-lost")
 	require.True(t, ok)
-	assert.Equal(t, encodeBuildRef("test-org", "my-pipeline", 7), ref)
+	assert.Equal(t, encodeBuildRef(7), ref)
 }
 
 func TestStatus_BuildkiteNotFound(t *testing.T) {
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusNotFound)
 	}))
-	r.storeRef("some-id", encodeBuildRef("test-org", "my-pipeline", 99))
+	r.storeRef("some-id", encodeBuildRef(99))
 
 	_, _, err := r.Status(context.Background(), entity.BuildID{ID: "some-id"})
 	require.Error(t, err)
@@ -391,7 +366,7 @@ func TestCancel_CallsBuildkiteWhenRefKnown(t *testing.T) {
 		w.WriteHeader(http.StatusOK)
 		_, _ = w.Write(buildJSON(5, "canceled", ""))
 	}))
-	r.storeRef("some-id", encodeBuildRef("test-org", "my-pipeline", 5))
+	r.storeRef("some-id", encodeBuildRef(5))
 
 	require.NoError(t, r.Cancel(context.Background(), entity.BuildID{ID: "some-id"}))
 	drainCancel(t, r)
@@ -435,15 +410,15 @@ func TestCancel_AlreadyTerminal_Noop(t *testing.T) {
 	r := newTestRunner(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusUnprocessableEntity)
 	}))
-	r.storeRef("some-id", encodeBuildRef("test-org", "my-pipeline", 5))
+	r.storeRef("some-id", encodeBuildRef(5))
 
 	require.NoError(t, r.Cancel(context.Background(), entity.BuildID{ID: "some-id"}))
 	drainCancel(t, r) // must not panic or error
 }
 
 func TestCancel_QueueFull_ReturnsError(t *testing.T) {
 	r := newRunner(
-		Config{OrgSlug: "org", PipelineSlug: "p", Branch: "main"},
+		Config{},
 		&client{httpClient: http.DefaultClient},
 		1, 1,
 	)
@@ -454,29 +429,18 @@ func TestCancel_QueueFull_ReturnsError(t *testing.T) {
 // --- Internal helpers ---
 
 func TestEncodeParseBuildRef_RoundTrip(t *testing.T) {
-	tests := []struct {
-		org      string
-		pipeline string
-		number   int
-	}{
-		{"myorg", "my-pipeline", 1},
-		{"uber", "submit-queue-ci", 9999},
-		{"a", "b", 0},
-	}
-	for _, tt := range tests {
-		ref := encodeBuildRef(tt.org, tt.pipeline, tt.number)
-		org, pipeline, number, err := parseBuildRef(ref)
+	for _, n := range []int{1, 9999, 0} {
+		ref := encodeBuildRef(n)
+		got, err := parseBuildRef(ref)
 		require.NoError(t, err)
-		assert.Equal(t, tt.org, org)
-		assert.Equal(t, tt.pipeline, pipeline)
-		assert.Equal(t, tt.number, number)
+		assert.Equal(t, n, got)
 	}
 }
 
 func TestParseBuildRef_Invalid(t *testing.T) {
-	for _, ref := range []string{"", "noslash", "only/one", "org/pipeline/notanumber"} {
+	for _, ref := range []string{"", "notanumber", "org/pipeline/1"} {
 		t.Run(ref, func(t *testing.T) {
-			_, _, _, err := parseBuildRef(ref)
+			_, err := parseBuildRef(ref)
 			require.Error(t, err)
 		})
 	}