Merge branch 'main' into manjari/batch-manager-gen-id

Author: manjari25

CLAUDE.md

@@ -184,8 +184,64 @@ make clean              # Clean Bazel cache
 **Add new entity:**
 1. Create `entity/{domain}/{entity}.go` with test file and `BUILD.bazel`
 
+**Add gomock for an extension interface:**
+
+Mocks use the Bazel `gomock` rule from `@rules_go//extras:gomock.bzl` and are generated at build time (not checked in). See `extension/storage/mock/` for the canonical example.
+
+To add a mock for a new interface file in an existing mock package (e.g., `extension/storage/new_store.go`):
+
+1. Add a `//go:generate` directive to the interface file:
+   ```go
+   //go:generate mockgen -source=new_store.go -destination=mock/new_store.go -package=mock
+   ```
+2. Add the file to `exports_files` in the parent `BUILD.bazel` with visibility to the mock package:
+   ```starlark
+   exports_files(
+       ["new_store.go", ...],
+       visibility = ["//extension/storage/mock:__pkg__"],
+   )
+   ```
+3. Add a `gomock` rule in the mock `BUILD.bazel`:
+   ```starlark
+   gomock(
+       name = "mock_new_store_src",
+       out = "new_store_mock.go",
+       mockgen_tool = _MOCKGEN,
+       package = "mock",
+       source = "//extension/storage:new_store.go",
+       source_importpath = "github.com/uber/submitqueue/extension/storage",
+   )
+   ```
+4. Add the rule target to the `go_library` srcs in the same file.
+
+To create a mock package for a new extension (e.g., `extension/queue/mock/`):
+
+1. Create `extension/{ext}/mock/BUILD.bazel` with `gomock` rules, a `go_library`, and `# gazelle:ignore`.
+2. Add `exports_files` to `extension/{ext}/BUILD.bazel` for each interface file.
+3. Follow the same per-interface pattern as above.
+
+Mock `BUILD.bazel` files use `# gazelle:ignore` so `make gazelle` will not update them — they must be maintained manually.
+
+**Using mocks in tests:**
+```go
+import storagemock "github.com/uber/submitqueue/extension/storage/mock"
+
+ctrl := gomock.NewController(t)
+mockStore := storagemock.NewMockRequestStore(ctrl)
+mockStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil)
+```
+
+Test `BUILD.bazel` deps:
+```starlark
+deps = [
+    "//extension/storage/mock",
+    "@org_uber_go_mock//gomock",
+]
+```
+
 ### Testing
 
+- **Table-driven tests** — prefer table-driven tests with `t.Run` subtests over individual test functions.
 - **Avoid asserting on error messages** — assert on error type or generic error.
 - **No change detector tests** — don't assert on default values, internal structure, or implementation details that can change without affecting behavior. Test what the code *does*, not how it's constructed.
 - **No `time.Sleep` for synchronization** — use channels, callbacks, condition variables.

entity/github/BUILD.bazel

@@ -0,0 +1,18 @@
+load("@rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "github",
+    srcs = ["change_id.go"],
+    importpath = "github.com/uber/submitqueue/entity/github",
+    visibility = ["//visibility:public"],
+)
+
+go_test(
+    name = "github_test",
+    srcs = ["change_id_test.go"],
+    embed = [":github"],
+    deps = [
+        "@com_github_stretchr_testify//assert",
+        "@com_github_stretchr_testify//require",
+    ],
+)

entity/github/change_id.go

@@ -0,0 +1,99 @@
+package github
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+)
+
+// changeIDFormat is the expected format for change IDs, included in error messages.
+const changeIDFormat = "{scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha}"
+
+// ChangeID represents a parsed GitHub-family change identifier.
+// Covers GitHub.com, GitHub Enterprise (GHE), and GitHub Enterprise Server (GHES)
+// since they share the same pull request model.
+// Format: {scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha}
+type ChangeID struct {
+	// Scheme captures the source variant (e.g., "github", "ghe", "ghes").
+	Scheme string
+	// Org is the organization or owner of the repository.
+	Org string
+	// Repo is the repository name.
+	Repo string
+	// PRNumber is the pull request number.
+	PRNumber int
+	// HeadCommitSHA is the head commit SHA at the time of request creation.
+	HeadCommitSHA string
+}
+
+// ParseChangeID parses a raw change ID string into a ChangeID.
+// Expected format: {scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha}
+// The parser works from the end: SHA (last), PR number (second-to-last),
+// and everything before is the repo path (split into owner and repo).
+func ParseChangeID(raw string) (ChangeID, error) {
+	// Split on "://" to get scheme and path
+	schemeSplit := strings.SplitN(raw, "://", 2)
+	if len(schemeSplit) != 2 {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: missing '://' separator (expected format: %s)", raw, changeIDFormat)
+	}
+
+	scheme := schemeSplit[0]
+	if scheme == "" {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: empty scheme (expected format: %s)", raw, changeIDFormat)
+	}
+
+	path := schemeSplit[1]
+
+	// Split the path into segments and parse from the end.
+	segments := strings.Split(path, "/")
+	// Need at least 4 segments: {owner}/{repo}/{pr_number}/{sha}
+	if len(segments) < 4 {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: need at least owner/repo/pr/sha, got %d segments (expected format: %s)", raw, len(segments), changeIDFormat)
+	}
+
+	sha := segments[len(segments)-1]
+	prStr := segments[len(segments)-2]
+	repoSegments := segments[:len(segments)-2]
+
+	if sha == "" {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: empty head commit SHA (expected format: %s)", raw, changeIDFormat)
+	}
+
+	prNumber, err := strconv.Atoi(prStr)
+	if err != nil {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: PR number %q is not a valid integer (expected format: %s)", raw, prStr, changeIDFormat)
+	}
+
+	// Split repo path: last segment is repo name, everything before is the owner.
+	if len(repoSegments) < 2 {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: repo path must have at least owner/repo (expected format: %s)", raw, changeIDFormat)
+	}
+
+	repo := repoSegments[len(repoSegments)-1]
+	org := strings.Join(repoSegments[:len(repoSegments)-1], "/")
+
+	if org == "" {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: empty owner (expected format: %s)", raw, changeIDFormat)
+	}
+	if repo == "" {
+		return ChangeID{}, fmt.Errorf("invalid change ID %q: empty repo (expected format: %s)", raw, changeIDFormat)
+	}
+
+	return ChangeID{
+		Scheme:        scheme,
+		Org:           org,
+		Repo:          repo,
+		PRNumber:      prNumber,
+		HeadCommitSHA: sha,
+	}, nil
+}
+
+// String returns the string representation of the change ID.
+func (c ChangeID) String() string {
+	return fmt.Sprintf("%s://%s/%s/%d/%s", c.Scheme, c.Org, c.Repo, c.PRNumber, c.HeadCommitSHA)
+}
+
+// OwnerRepo returns the "{org}/{repo}" string.
+func (c ChangeID) OwnerRepo() string {
+	return fmt.Sprintf("%s/%s", c.Org, c.Repo)
+}

entity/github/change_id_test.go

@@ -0,0 +1,194 @@
+package github
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseChangeID(t *testing.T) {
+	tests := []struct {
+		name    string
+		raw     string
+		want    ChangeID
+		wantErr bool
+	}{
+		{
+			name: "valid github scheme",
+			raw:  "github://uber/submitqueue/123/abc123def",
+			want: ChangeID{
+				Scheme:        "github",
+				Org:           "uber",
+				Repo:          "submitqueue",
+				PRNumber:      123,
+				HeadCommitSHA: "abc123def",
+			},
+		},
+		{
+			name: "valid ghe scheme",
+			raw:  "ghe://uber/monorepo/456/deadbeef",
+			want: ChangeID{
+				Scheme:        "ghe",
+				Org:           "uber",
+				Repo:          "monorepo",
+				PRNumber:      456,
+				HeadCommitSHA: "deadbeef",
+			},
+		},
+		{
+			name: "valid ghes scheme",
+			raw:  "ghes://org/repo/1/sha1",
+			want: ChangeID{
+				Scheme:        "ghes",
+				Org:           "org",
+				Repo:          "repo",
+				PRNumber:      1,
+				HeadCommitSHA: "sha1",
+			},
+		},
+		{
+			name: "nested org path",
+			raw:  "github://uber/frontend/webapp/42/abc123",
+			want: ChangeID{
+				Scheme:        "github",
+				Org:           "uber/frontend",
+				Repo:          "webapp",
+				PRNumber:      42,
+				HeadCommitSHA: "abc123",
+			},
+		},
+		{
+			name:    "missing separator",
+			raw:     "github/uber/submitqueue/123/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "empty scheme",
+			raw:     "://uber/submitqueue/123/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "too few segments",
+			raw:     "github://uber/123/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "only one segment",
+			raw:     "github://abc123",
+			wantErr: true,
+		},
+		{
+			name:    "empty owner",
+			raw:     "github:///submitqueue/123/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "empty repo",
+			raw:     "github://uber//123/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "non-numeric PR number",
+			raw:     "github://uber/submitqueue/abc/abc123",
+			wantErr: true,
+		},
+		{
+			name:    "empty SHA",
+			raw:     "github://uber/submitqueue/123/",
+			wantErr: true,
+		},
+		{
+			name:    "empty string",
+			raw:     "",
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ParseChangeID(tt.raw)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestChangeID_String(t *testing.T) {
+	tests := []struct {
+		name string
+		id   ChangeID
+		want string
+	}{
+		{
+			name: "github",
+			id: ChangeID{
+				Scheme:        "github",
+				Org:           "uber",
+				Repo:          "submitqueue",
+				PRNumber:      123,
+				HeadCommitSHA: "abc123",
+			},
+			want: "github://uber/submitqueue/123/abc123",
+		},
+		{
+			name: "ghe",
+			id: ChangeID{
+				Scheme:        "ghe",
+				Org:           "corp",
+				Repo:          "app",
+				PRNumber:      99,
+				HeadCommitSHA: "deadbeef",
+			},
+			want: "ghe://corp/app/99/deadbeef",
+		},
+		{
+			name: "ghes",
+			id: ChangeID{
+				Scheme:        "ghes",
+				Org:           "org",
+				Repo:          "repo",
+				PRNumber:      1,
+				HeadCommitSHA: "sha1",
+			},
+			want: "ghes://org/repo/1/sha1",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.id.String())
+		})
+	}
+}
+
+func TestChangeID_OwnerRepo(t *testing.T) {
+	id := ChangeID{
+		Scheme:        "github",
+		Org:           "uber",
+		Repo:          "submitqueue",
+		PRNumber:      1,
+		HeadCommitSHA: "abc",
+	}
+	assert.Equal(t, "uber/submitqueue", id.OwnerRepo())
+}
+
+func TestParseChangeID_RoundTrip(t *testing.T) {
+	originals := []string{
+		"github://uber/submitqueue/123/abc123def456",
+		"ghe://corp/monorepo/99/deadbeef01234567",
+		"ghes://org/repo/1/a1b2c3",
+	}
+
+	for _, raw := range originals {
+		t.Run(raw, func(t *testing.T) {
+			parsed, err := ParseChangeID(raw)
+			require.NoError(t, err)
+			assert.Equal(t, raw, parsed.String())
+		})
+	}
+}

entity/queue_config.go

@@ -33,4 +33,19 @@ type QueueConfig struct {
 	//   - Buildkite: "buildkite.com/uber/submitqueue-ci"
 	//   - Jenkins: "jenkins.example.com/job/submitqueue-verify"
 	BuildRunner string `json:"build_runner" yaml:"build_runner"`
+
+	// ChangeProvider identifies the change provider implementation for this queue.
+	// Opaque to the system; meaningful only to the change provider extension implementation.
+	// Examples: "github", "gitlab", "phabricator"
+	ChangeProvider string `json:"change_provider" yaml:"change_provider"`
+
+	// MergeChecker identifies the merge checker implementation for this queue.
+	// Opaque to the system; meaningful only to the merge checker extension implementation.
+	// Examples: "github", "gitlab"
+	MergeChecker string `json:"merge_checker" yaml:"merge_checker"`
+
+	// LandProvider identifies the land provider implementation for this queue.
+	// Opaque to the system; meaningful only to the land provider extension implementation.
+	// Examples: "github", "gitlab"
+	LandProvider string `json:"land_provider" yaml:"land_provider"`
 }