Skip to content

Commit 3cebc2e

Browse files
authored
refactor(mergechecker): accept entity.Request, resolve change internally (#217)
## Summary 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. ## Test Plan ## Issues ## Stack 1. @ #217 1. #218 1. #219 1. #221 1. #222 1. #223 1. #227
1 parent 679143c commit 3cebc2e

7 files changed

Lines changed: 18 additions & 15 deletions

File tree

submitqueue/extension/mergechecker/fake/fake.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,9 @@ func New() mergechecker.MergeChecker {
5252
}
5353

5454
// Check reports the change as mergeable unless a recognized marker token is
55-
// present in one of its URIs.
56-
func (checker) Check(_ context.Context, change entity.Change) (mergechecker.Result, error) {
57-
switch fakemarker.Token(change.URIs) {
55+
// present in one of the request's change URIs.
56+
func (checker) Check(_ context.Context, request entity.Request) (mergechecker.Result, error) {
57+
switch fakemarker.Token(request.Change.URIs) {
5858
case tokenUnmergeable:
5959
return mergechecker.Result{Mergeable: false, Reason: "fake: marked unmergeable"}, nil
6060
case tokenError:

submitqueue/extension/mergechecker/fake/fake_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ func TestChecker_Check(t *testing.T) {
6666
c := New()
6767
for _, tt := range tests {
6868
t.Run(tt.name, func(t *testing.T) {
69-
res, err := c.Check(context.Background(), entity.Change{URIs: tt.uris})
69+
res, err := c.Check(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}})
7070
if tt.wantErr {
7171
require.Error(t, err)
7272
return

submitqueue/extension/mergechecker/github/checker.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,15 @@ func NewMergeChecker(params Params) mergechecker.MergeChecker {
6060
}
6161
}
6262

63-
// Check assesses whether a change can merge cleanly using the GitHub GraphQL API.
64-
func (c *mergeChecker) Check(ctx context.Context, change entity.Change) (result mergechecker.Result, retErr error) {
63+
// Check assesses whether a request's change can merge cleanly using the GitHub GraphQL API.
64+
func (c *mergeChecker) Check(ctx context.Context, request entity.Request) (result mergechecker.Result, retErr error) {
6565
const opName = "check"
6666

6767
op := metrics.Begin(c.metricsScope, opName)
6868
defer func() { op.Complete(retErr) }()
6969

70+
change := request.Change
71+
7072
// Parse all change IDs
7173
// TODO: classify parse errors as user errors (non-retryable) vs system errors.
7274
changeIDs := make([]entitygithub.ChangeID, 0, len(change.URIs))

submitqueue/extension/mergechecker/github/checker_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ func TestMergeChecker_Check(t *testing.T) {
156156
defer server.Close()
157157

158158
mc := newTestMergeChecker(t, server.URL)
159-
result, err := mc.Check(context.Background(), tt.change)
159+
result, err := mc.Check(context.Background(), entity.Request{Change: tt.change})
160160
if tt.wantErr {
161161
require.Error(t, err)
162162
return

submitqueue/extension/mergechecker/mergechecker.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,13 @@ import (
2222
"github.com/uber/submitqueue/submitqueue/entity"
2323
)
2424

25-
// MergeChecker predicts whether a set of changes can merge cleanly.
25+
// MergeChecker predicts whether a request's changes can merge cleanly.
2626
type MergeChecker interface {
2727
// Check is a fail-fast mergeability check that optimistically assesses
28-
// whether the changes can be merged. A positive result does not
28+
// whether the request's changes can be merged. It is handed the request
29+
// identity and reads request.Change itself. A positive result does not
2930
// guarantee that the changes will apply cleanly at merge time.
30-
Check(ctx context.Context, change entity.Change) (Result, error)
31+
Check(ctx context.Context, request entity.Request) (Result, error)
3132
}
3233

3334
// Result holds the outcome of a mergeability check.

submitqueue/extension/mergechecker/mock/mergechecker_mock.go

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

submitqueue/orchestrator/controller/validate/validate.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
140140
coremetrics.NamedCounter(c.metricsScope, "process", "merge_check_errors", 1)
141141
return fmt.Errorf("failed to build merge checker for queue %s: %w", request.Queue, err)
142142
}
143-
mergeResult, err := mergeChecker.Check(ctx, request.Change)
143+
mergeResult, err := mergeChecker.Check(ctx, request)
144144
if err != nil {
145145
coremetrics.NamedCounter(c.metricsScope, "process", "merge_check_errors", 1)
146146
return fmt.Errorf("merge check failed: %w", err)

0 commit comments

Comments
 (0)