From 80b274dc7dbd0c9b5dfa90bc8b3059333c154ed4 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 8 Jun 2026 11:48:44 -0700 Subject: [PATCH 1/2] refactor(mergechecker): accept entity.Request, resolve change internally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change MergeChecker.Check to take the orchestrator's request identity (entity.Request) instead of a controller-pre-resolved entity.Change, per the extension contract. The GitHub implementation and the fake read request.Change themselves; the validate controller hands over the request it already loaded. Output is unchanged (mergechecker.Result). The factory and Config are unchanged — no dependency injection is needed since the checker resolves nothing beyond the change already on the request. --- submitqueue/extension/mergechecker/fake/fake.go | 6 +++--- submitqueue/extension/mergechecker/fake/fake_test.go | 2 +- submitqueue/extension/mergechecker/github/checker.go | 6 ++++-- submitqueue/extension/mergechecker/github/checker_test.go | 2 +- submitqueue/extension/mergechecker/mergechecker.go | 7 ++++--- .../extension/mergechecker/mock/mergechecker_mock.go | 8 ++++---- submitqueue/orchestrator/controller/validate/validate.go | 2 +- 7 files changed, 18 insertions(+), 15 deletions(-) diff --git a/submitqueue/extension/mergechecker/fake/fake.go b/submitqueue/extension/mergechecker/fake/fake.go index 1fd6867f..c66165ab 100644 --- a/submitqueue/extension/mergechecker/fake/fake.go +++ b/submitqueue/extension/mergechecker/fake/fake.go @@ -52,9 +52,9 @@ func New() mergechecker.MergeChecker { } // Check reports the change as mergeable unless a recognized marker token is -// present in one of its URIs. -func (checker) Check(_ context.Context, change entity.Change) (mergechecker.Result, error) { - switch fakemarker.Token(change.URIs) { +// present in one of the request's change URIs. +func (checker) Check(_ context.Context, request entity.Request) (mergechecker.Result, error) { + switch fakemarker.Token(request.Change.URIs) { case tokenUnmergeable: return mergechecker.Result{Mergeable: false, Reason: "fake: marked unmergeable"}, nil case tokenError: diff --git a/submitqueue/extension/mergechecker/fake/fake_test.go b/submitqueue/extension/mergechecker/fake/fake_test.go index f55a9d17..142b8dc1 100644 --- a/submitqueue/extension/mergechecker/fake/fake_test.go +++ b/submitqueue/extension/mergechecker/fake/fake_test.go @@ -66,7 +66,7 @@ func TestChecker_Check(t *testing.T) { c := New() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - res, err := c.Check(context.Background(), entity.Change{URIs: tt.uris}) + res, err := c.Check(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}}) if tt.wantErr { require.Error(t, err) return diff --git a/submitqueue/extension/mergechecker/github/checker.go b/submitqueue/extension/mergechecker/github/checker.go index 7fb2dc92..4a5134aa 100644 --- a/submitqueue/extension/mergechecker/github/checker.go +++ b/submitqueue/extension/mergechecker/github/checker.go @@ -60,13 +60,15 @@ func NewMergeChecker(params Params) mergechecker.MergeChecker { } } -// Check assesses whether a change can merge cleanly using the GitHub GraphQL API. -func (c *mergeChecker) Check(ctx context.Context, change entity.Change) (result mergechecker.Result, retErr error) { +// Check assesses whether a request's change can merge cleanly using the GitHub GraphQL API. +func (c *mergeChecker) Check(ctx context.Context, request entity.Request) (result mergechecker.Result, retErr error) { const opName = "check" op := metrics.Begin(c.metricsScope, opName) defer func() { op.Complete(retErr) }() + change := request.Change + // Parse all change IDs // TODO: classify parse errors as user errors (non-retryable) vs system errors. changeIDs := make([]entitygithub.ChangeID, 0, len(change.URIs)) diff --git a/submitqueue/extension/mergechecker/github/checker_test.go b/submitqueue/extension/mergechecker/github/checker_test.go index f1089a4b..7369d65d 100644 --- a/submitqueue/extension/mergechecker/github/checker_test.go +++ b/submitqueue/extension/mergechecker/github/checker_test.go @@ -156,7 +156,7 @@ func TestMergeChecker_Check(t *testing.T) { defer server.Close() mc := newTestMergeChecker(t, server.URL) - result, err := mc.Check(context.Background(), tt.change) + result, err := mc.Check(context.Background(), entity.Request{Change: tt.change}) if tt.wantErr { require.Error(t, err) return diff --git a/submitqueue/extension/mergechecker/mergechecker.go b/submitqueue/extension/mergechecker/mergechecker.go index 50ef5d09..28c95d8e 100644 --- a/submitqueue/extension/mergechecker/mergechecker.go +++ b/submitqueue/extension/mergechecker/mergechecker.go @@ -22,12 +22,13 @@ import ( "github.com/uber/submitqueue/submitqueue/entity" ) -// MergeChecker predicts whether a set of changes can merge cleanly. +// MergeChecker predicts whether a request's changes can merge cleanly. type MergeChecker interface { // Check is a fail-fast mergeability check that optimistically assesses - // whether the changes can be merged. A positive result does not + // whether the request's changes can be merged. It is handed the request + // identity and reads request.Change itself. A positive result does not // guarantee that the changes will apply cleanly at merge time. - Check(ctx context.Context, change entity.Change) (Result, error) + Check(ctx context.Context, request entity.Request) (Result, error) } // Result holds the outcome of a mergeability check. diff --git a/submitqueue/extension/mergechecker/mock/mergechecker_mock.go b/submitqueue/extension/mergechecker/mock/mergechecker_mock.go index eab22c58..08f28f8c 100644 --- a/submitqueue/extension/mergechecker/mock/mergechecker_mock.go +++ b/submitqueue/extension/mergechecker/mock/mergechecker_mock.go @@ -43,18 +43,18 @@ func (m *MockMergeChecker) EXPECT() *MockMergeCheckerMockRecorder { } // Check mocks base method. -func (m *MockMergeChecker) Check(ctx context.Context, change entity.Change) (mergechecker.Result, error) { +func (m *MockMergeChecker) Check(ctx context.Context, request entity.Request) (mergechecker.Result, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Check", ctx, change) + ret := m.ctrl.Call(m, "Check", ctx, request) ret0, _ := ret[0].(mergechecker.Result) ret1, _ := ret[1].(error) return ret0, ret1 } // Check indicates an expected call of Check. -func (mr *MockMergeCheckerMockRecorder) Check(ctx, change any) *gomock.Call { +func (mr *MockMergeCheckerMockRecorder) Check(ctx, request any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Check", reflect.TypeOf((*MockMergeChecker)(nil).Check), ctx, change) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Check", reflect.TypeOf((*MockMergeChecker)(nil).Check), ctx, request) } // MockFactory is a mock of Factory interface. diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 5a44309f..123d0556 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -140,7 +140,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r coremetrics.NamedCounter(c.metricsScope, "process", "merge_check_errors", 1) return fmt.Errorf("failed to build merge checker for queue %s: %w", request.Queue, err) } - mergeResult, err := mergeChecker.Check(ctx, request.Change) + mergeResult, err := mergeChecker.Check(ctx, request) if err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "merge_check_errors", 1) return fmt.Errorf("merge check failed: %w", err) From 985169f7c70bc0cbd54c12f78d628b61ccf086b9 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 8 Jun 2026 11:52:17 -0700 Subject: [PATCH 2/2] refactor(changeprovider): accept entity.Request, resolve change internally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change ChangeProvider.Get to take the orchestrator's request identity (entity.Request) instead of a controller-pre-resolved entity.Change, per the extension contract. The GitHub implementation and the fake read request.Change themselves; the validate controller hands over the request it already loaded. Output is unchanged: one entity.ChangeInfo per URI, each self-identifying by URI. The provider is the external resolver, so it needs no injected dependency — the factory and Config are unchanged. --- .../extension/changeprovider/change_provider.go | 5 +++-- submitqueue/extension/changeprovider/fake/fake.go | 7 ++++--- .../extension/changeprovider/fake/fake_test.go | 6 +++--- .../extension/changeprovider/github/provider.go | 6 ++++-- .../changeprovider/github/provider_test.go | 14 +++++++------- .../changeprovider/mock/change_provider_mock.go | 8 ++++---- .../orchestrator/controller/validate/validate.go | 2 +- .../controller/validate/validate_test.go | 2 +- 8 files changed, 27 insertions(+), 23 deletions(-) diff --git a/submitqueue/extension/changeprovider/change_provider.go b/submitqueue/extension/changeprovider/change_provider.go index 9b4b7433..ba55b8bf 100644 --- a/submitqueue/extension/changeprovider/change_provider.go +++ b/submitqueue/extension/changeprovider/change_provider.go @@ -29,10 +29,11 @@ import ( // entity.Author, entity.ChangedFile — live in the entity package so the same typed // facts can be persisted (entity.ChangeRecord) and scored without re-declaration. type ChangeProvider interface { - // Get retrieves change information for the provided Change. + // Get retrieves change information for the provided request. + // It is handed the request identity and reads request.Change itself. // For a Change with multiple URIs (e.g., stacked PRs), returns one ChangeInfo per URI. // Returns a slice of ChangeInfo, one for each change in the stack. - Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) + Get(ctx context.Context, request entity.Request) ([]entity.ChangeInfo, error) } // Config carries the per-queue identity handed to a Factory. The system knows diff --git a/submitqueue/extension/changeprovider/fake/fake.go b/submitqueue/extension/changeprovider/fake/fake.go index 6d829c3a..1e30998e 100644 --- a/submitqueue/extension/changeprovider/fake/fake.go +++ b/submitqueue/extension/changeprovider/fake/fake.go @@ -47,9 +47,10 @@ func New() changeprovider.ChangeProvider { return provider{} } -// Get returns one ChangeInfo per URI in the change, unless a recognized marker -// token requests a failure. The "one ChangeInfo per URI" contract is preserved. -func (provider) Get(_ context.Context, change entity.Change) ([]entity.ChangeInfo, error) { +// Get returns one ChangeInfo per URI in the request's change, unless a recognized +// marker token requests a failure. The "one ChangeInfo per URI" contract is preserved. +func (provider) Get(_ context.Context, request entity.Request) ([]entity.ChangeInfo, error) { + change := request.Change if fakemarker.Token(change.URIs) == tokenError { return nil, fmt.Errorf("fake: marked provider error") } diff --git a/submitqueue/extension/changeprovider/fake/fake_test.go b/submitqueue/extension/changeprovider/fake/fake_test.go index bb7c40db..a0a4fb2f 100644 --- a/submitqueue/extension/changeprovider/fake/fake_test.go +++ b/submitqueue/extension/changeprovider/fake/fake_test.go @@ -47,7 +47,7 @@ func TestProvider_Get_OnePerURI(t *testing.T) { p := New() for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - infos, err := p.Get(context.Background(), entity.Change{URIs: tt.uris}) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}}) require.NoError(t, err) require.Len(t, infos, len(tt.uris)) for i, uri := range tt.uris { @@ -59,8 +59,8 @@ func TestProvider_Get_OnePerURI(t *testing.T) { func TestProvider_Get_ErrorMarker(t *testing.T) { p := New() - _, err := p.Get(context.Background(), entity.Change{ + _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ URIs: []string{"github://owner/repo/pull/1/abc?sq-fake=provider-error"}, - }) + }}) require.Error(t, err) } diff --git a/submitqueue/extension/changeprovider/github/provider.go b/submitqueue/extension/changeprovider/github/provider.go index 15daaa6a..5b1bcf89 100644 --- a/submitqueue/extension/changeprovider/github/provider.go +++ b/submitqueue/extension/changeprovider/github/provider.go @@ -42,12 +42,14 @@ func NewProvider(params Params) changeprovider.ChangeProvider { } } -// Get retrieves change information from GitHub for the provided Change. +// Get retrieves change information from GitHub for the request's change. // Returns one ChangeInfo per URI (one per PR in stacked changes). -func (p *provider) Get(ctx context.Context, change entity.Change) (_ []entity.ChangeInfo, retErr error) { +func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.ChangeInfo, retErr error) { op := coremetrics.Begin(p.metricsScope, "get") defer func() { op.Complete(retErr) }() + change := request.Change + // Parse all change IDs changeIDs := make([]entitygithub.ChangeID, 0, len(change.URIs)) for _, uri := range change.URIs { diff --git a/submitqueue/extension/changeprovider/github/provider_test.go b/submitqueue/extension/changeprovider/github/provider_test.go index def8e767..7907ec98 100644 --- a/submitqueue/extension/changeprovider/github/provider_test.go +++ b/submitqueue/extension/changeprovider/github/provider_test.go @@ -106,7 +106,7 @@ func TestProvider_Get(t *testing.T) { } p := newTestProvider(t, serverURL) - infos, err := p.Get(context.Background(), entity.Change{URIs: tt.uris}) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}}) if tt.wantErr { require.Error(t, err) @@ -147,9 +147,9 @@ func TestProvider_Get_Pagination(t *testing.T) { defer server.Close() p := newTestProvider(t, server.URL) - infos, err := p.Get(context.Background(), entity.Change{ + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ URIs: []string{"github://uber/submitqueue/pull/456/" + shaXYZ}, - }) + }}) require.NoError(t, err) assert.Equal(t, 2, callCount) @@ -170,12 +170,12 @@ func TestProvider_Get_MultiplePRs(t *testing.T) { defer server.Close() p := newTestProvider(t, server.URL) - infos, err := p.Get(context.Background(), entity.Change{ + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ URIs: []string{ "github://uber/submitqueue/pull/123/" + shaA, "github://uber/submitqueue/pull/456/" + shaB, }, - }) + }}) require.NoError(t, err) assert.Equal(t, 2, callCount) @@ -202,12 +202,12 @@ func TestProvider_Get_FetchError_StopsOnFirstFailure(t *testing.T) { defer server.Close() p := newTestProvider(t, server.URL) - _, err := p.Get(context.Background(), entity.Change{ + _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ URIs: []string{ "github://uber/submitqueue/pull/123/" + shaA, "github://uber/submitqueue/pull/456/" + shaB, }, - }) + }}) require.Error(t, err) assert.Equal(t, 2, callCount) diff --git a/submitqueue/extension/changeprovider/mock/change_provider_mock.go b/submitqueue/extension/changeprovider/mock/change_provider_mock.go index a1afc4ae..4440e6e8 100644 --- a/submitqueue/extension/changeprovider/mock/change_provider_mock.go +++ b/submitqueue/extension/changeprovider/mock/change_provider_mock.go @@ -43,18 +43,18 @@ func (m *MockChangeProvider) EXPECT() *MockChangeProviderMockRecorder { } // Get mocks base method. -func (m *MockChangeProvider) Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) { +func (m *MockChangeProvider) Get(ctx context.Context, request entity.Request) ([]entity.ChangeInfo, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", ctx, change) + ret := m.ctrl.Call(m, "Get", ctx, request) ret0, _ := ret[0].([]entity.ChangeInfo) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockChangeProviderMockRecorder) Get(ctx, change any) *gomock.Call { +func (mr *MockChangeProviderMockRecorder) Get(ctx, request any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockChangeProvider)(nil).Get), ctx, change) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockChangeProvider)(nil).Get), ctx, request) } // MockFactory is a mock of Factory interface. diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 123d0556..ad7d1c31 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -161,7 +161,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r coremetrics.NamedCounter(c.metricsScope, "process", "change_provider_errors", 1) return fmt.Errorf("failed to build change provider for queue %s: %w", request.Queue, err) } - changeInfos, err := changeProvider.Get(ctx, request.Change) + changeInfos, err := changeProvider.Get(ctx, request) if err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "change_provider_errors", 1) return fmt.Errorf("failed to fetch change information for request %s: %w", request.ID, err) diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index 20f4804f..90e98939 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -46,7 +46,7 @@ func requestIDPayload(t *testing.T, id string) []byte { // mockChangeProvider is a simple mock that returns test data. type mockChangeProvider struct{} -func (m *mockChangeProvider) Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) { +func (m *mockChangeProvider) Get(ctx context.Context, request entity.Request) ([]entity.ChangeInfo, error) { return []entity.ChangeInfo{ { URI: "github://org/repo/pull/123/abcdef0123456789abcdef0123456789abcdef01",