From 80b274dc7dbd0c9b5dfa90bc8b3059333c154ed4 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 8 Jun 2026 11:48:44 -0700 Subject: [PATCH] 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)