Skip to content

Commit 99f422d

Browse files
authored
refactor(entity): relocate analyzer/checker/pusher result types to entity (#227)
## Summary Move the domain-fact data types produced by the conflict, mergechecker, and pusher extensions into entity/submitqueue, leaving each extension with only its behavioral contract (interface, Config, Factory) and sentinels: - conflict.Conflict / ConflictType -> entity.Conflict / entity.ConflictType - mergechecker.Result -> entity.MergeResult - pusher.{Result,BatchOutcome,ChangeOutcome,OutcomeStatus} -> entity.{PushResult,BatchOutcome,ChangeOutcome,OutcomeStatus} These are domain facts (a dependency-graph reason, a mergeability verdict, the commits a change produced) that the orchestrator may persist or surface. Placing them in entity/ — the universal dependency sink that storage, controllers, and extensions all already import — keeps the contract robust to future persistence without a layering inversion (storage importing a decision extension) or an import migration later. This matches what buildrunner and changeprovider already do with entity.BuildStatus / entity.ChangeInfo. The data shapes are unchanged; only their home is. pusher.ErrConflict stays in the extension (a sentinel error is part of the behavioral contract). The controllers were already type-inference-clean, so only their tests change. ## Test Plan make build make test
1 parent ead06a1 commit 99f422d

29 files changed

Lines changed: 245 additions & 194 deletions

submitqueue/entity/BUILD.bazel

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ go_library(
1010
"cancel_request.go",
1111
"change_provider.go",
1212
"change_record.go",
13+
"conflict.go",
1314
"land_request.go",
15+
"merge_result.go",
16+
"push_result.go",
1417
"queue_config.go",
1518
"request.go",
1619
"request_log.go",

submitqueue/entity/conflict.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package entity
16+
17+
// ConflictType classifies why two batches are considered to conflict.
18+
// New values may be added as more sophisticated analyzers are introduced.
19+
type ConflictType string
20+
21+
const (
22+
// ConflictTypeUnknown is the unreachable zero value, set by default when
23+
// the structure is initialized. It should never be seen in the system.
24+
ConflictTypeUnknown ConflictType = ""
25+
// ConflictTypeConservative means the analyzer treated the batches as
26+
// conflicting because it could not prove otherwise, without identifying a
27+
// specific reason. Used by conservative analyzers that serialize
28+
// everything by default.
29+
ConflictTypeConservative ConflictType = "conservative"
30+
// ConflictTypeTargetOverlap means the two batches modify one or more of
31+
// the same build targets and may therefore interfere with each other.
32+
ConflictTypeTargetOverlap ConflictType = "target_overlap"
33+
)
34+
35+
// Conflict reports a single conflict between an analyzed batch and one of the
36+
// in-flight batches.
37+
type Conflict struct {
38+
// BatchID is the ID of the in-flight batch that conflicts with the
39+
// analyzed batch.
40+
BatchID string
41+
// Type classifies the conflict. A single (analyzed, in-flight) pair may
42+
// be reported with multiple Conflict entries when different conflict
43+
// types apply.
44+
Type ConflictType
45+
}

submitqueue/entity/merge_result.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package entity
16+
17+
// MergeResult holds the outcome of a mergeability check.
18+
type MergeResult struct {
19+
// Mergeable is true if the request's changes are expected to merge cleanly.
20+
Mergeable bool
21+
// Reason is a human-readable explanation when Mergeable is false.
22+
// Empty when Mergeable is true.
23+
Reason string
24+
}

submitqueue/entity/push_result.go

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package entity
16+
17+
// OutcomeStatus describes what happened to a single Change during a push.
18+
type OutcomeStatus string
19+
20+
const (
21+
// OutcomeStatusUnknown is the unreachable zero value, set by default
22+
// when the structure is initialized. It should never be seen in the system.
23+
OutcomeStatusUnknown OutcomeStatus = ""
24+
// OutcomeStatusCommitted means the change produced one or more commits
25+
// on the target branch. CommitSHAs lists those commits in apply order.
26+
OutcomeStatusCommitted OutcomeStatus = "committed"
27+
// OutcomeStatusAlreadyExisted means the change produced no commits
28+
// because every part of it is already present in the target branch
29+
// (e.g. it previously landed via another path, or a prior change in
30+
// the same push subsumed it). CommitSHAs is empty for this status.
31+
// In git terms this is what a `cherry-pick` surfaces as "rebased out".
32+
OutcomeStatusAlreadyExisted OutcomeStatus = "already_existed"
33+
)
34+
35+
// ChangeOutcome describes what happened to a single Change inside a push.
36+
type ChangeOutcome struct {
37+
// Change is the input change this outcome corresponds to.
38+
Change Change
39+
// Status describes whether the change produced commits or was already
40+
// present on the target branch.
41+
Status OutcomeStatus
42+
// CommitSHAs lists the commits this change produced on the target
43+
// branch, in apply order. A single Change may produce multiple commits
44+
// (e.g. a stack of PRs). Empty when Status is OutcomeStatusAlreadyExisted.
45+
CommitSHAs []string
46+
}
47+
48+
// BatchOutcome groups the per-change outcomes for a single pushed batch, so a
49+
// merge-train push (several batches in one call) stays correlatable back to the
50+
// batch each change belonged to. There is no per-batch status: a push is
51+
// all-or-nothing across the whole call, so a per-batch pass/fail would be
52+
// uniformly redundant.
53+
type BatchOutcome struct {
54+
// BatchID is the input batch this outcome corresponds to.
55+
BatchID string
56+
// Outcomes is one entry per change in the batch, in apply order.
57+
Outcomes []ChangeOutcome
58+
}
59+
60+
// PushResult is the outcome of a successful push.
61+
type PushResult struct {
62+
// Batches is one entry per pushed batch, in the same order as the batches
63+
// passed to the push. The slice length equals the input length.
64+
Batches []BatchOutcome
65+
}

submitqueue/extension/conflict/all/BUILD.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ go_test(
1717
embed = [":all"],
1818
deps = [
1919
"//submitqueue/entity",
20-
"//submitqueue/extension/conflict",
2120
"@com_github_stretchr_testify//assert",
2221
"@com_github_stretchr_testify//require",
2322
],

submitqueue/extension/conflict/all/all.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,15 @@ func New() conflict.Analyzer {
3636

3737
// Analyze returns one ConflictTypeConservative Conflict per in-flight batch,
3838
// preserving the input order. Returns an empty slice when inFlight is empty.
39-
func (analyzer) Analyze(_ context.Context, _ entity.Batch, inFlight []entity.Batch) ([]conflict.Conflict, error) {
39+
func (analyzer) Analyze(_ context.Context, _ entity.Batch, inFlight []entity.Batch) ([]entity.Conflict, error) {
4040
if len(inFlight) == 0 {
4141
return nil, nil
4242
}
43-
conflicts := make([]conflict.Conflict, len(inFlight))
43+
conflicts := make([]entity.Conflict, len(inFlight))
4444
for i, b := range inFlight {
45-
conflicts[i] = conflict.Conflict{
45+
conflicts[i] = entity.Conflict{
4646
BatchID: b.ID,
47-
Type: conflict.ConflictTypeConservative,
47+
Type: entity.ConflictTypeConservative,
4848
}
4949
}
5050
return conflicts, nil

submitqueue/extension/conflict/all/all_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"github.com/stretchr/testify/assert"
2222
"github.com/stretchr/testify/require"
2323
"github.com/uber/submitqueue/submitqueue/entity"
24-
"github.com/uber/submitqueue/submitqueue/extension/conflict"
2524
)
2625

2726
func TestAnalyze(t *testing.T) {
@@ -30,7 +29,7 @@ func TestAnalyze(t *testing.T) {
3029
tests := []struct {
3130
name string
3231
inFlight []entity.Batch
33-
want []conflict.Conflict
32+
want []entity.Conflict
3433
}{
3534
{
3635
name: "no in-flight batches yields no conflicts",
@@ -49,10 +48,10 @@ func TestAnalyze(t *testing.T) {
4948
{ID: "queueA/batch/2"},
5049
{ID: "queueA/batch/3"},
5150
},
52-
want: []conflict.Conflict{
53-
{BatchID: "queueA/batch/1", Type: conflict.ConflictTypeConservative},
54-
{BatchID: "queueA/batch/2", Type: conflict.ConflictTypeConservative},
55-
{BatchID: "queueA/batch/3", Type: conflict.ConflictTypeConservative},
51+
want: []entity.Conflict{
52+
{BatchID: "queueA/batch/1", Type: entity.ConflictTypeConservative},
53+
{BatchID: "queueA/batch/2", Type: entity.ConflictTypeConservative},
54+
{BatchID: "queueA/batch/3", Type: entity.ConflictTypeConservative},
5655
},
5756
},
5857
}

submitqueue/extension/conflict/conflict.go

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,36 +22,6 @@ import (
2222
"github.com/uber/submitqueue/submitqueue/entity"
2323
)
2424

25-
// ConflictType classifies why two batches are considered to conflict.
26-
// New values may be added as more sophisticated analyzers are introduced.
27-
type ConflictType string
28-
29-
const (
30-
// ConflictTypeUnknown is the unreachable zero value, set by default when
31-
// the structure is initialized. It should never be seen in the system.
32-
ConflictTypeUnknown ConflictType = ""
33-
// ConflictTypeConservative means the analyzer treated the batches as
34-
// conflicting because it could not prove otherwise, without identifying a
35-
// specific reason. Used by conservative analyzers that serialize
36-
// everything by default.
37-
ConflictTypeConservative ConflictType = "conservative"
38-
// ConflictTypeTargetOverlap means the two batches modify one or more of
39-
// the same build targets and may therefore interfere with each other.
40-
ConflictTypeTargetOverlap ConflictType = "target_overlap"
41-
)
42-
43-
// Conflict reports a single conflict between the analyzed batch and one of
44-
// the in-flight batches.
45-
type Conflict struct {
46-
// BatchID is the ID of the in-flight batch that conflicts with the
47-
// analyzed batch.
48-
BatchID string
49-
// Type classifies the conflict. A single (analyzed, in-flight) pair may
50-
// be reported with multiple Conflict entries when different conflict
51-
// types apply.
52-
Type ConflictType
53-
}
54-
5525
// Analyzer detects conflicts between a candidate batch and the batches
5626
// already in flight, so the speculation layer can decide which batches can
5727
// safely advance in parallel.
@@ -64,7 +34,7 @@ type Analyzer interface {
6434
// should be filtered out before calling. A non-nil error indicates the
6535
// analysis itself failed (infrastructure issue) and should be treated as
6636
// retryable by the caller.
67-
Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]Conflict, error)
37+
Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]entity.Conflict, error)
6838
}
6939

7040
// Config carries the per-queue identity handed to a Factory. The system knows

submitqueue/extension/conflict/fake/fake.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ func New(delegate conflict.Analyzer, failOn FailOn) conflict.Analyzer {
5454

5555
// Analyze returns an error when failOn reports true; otherwise it delegates to
5656
// the wrapped analyzer.
57-
func (a analyzerFake) Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]conflict.Conflict, error) {
57+
func (a analyzerFake) Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]entity.Conflict, error) {
5858
if a.failOn != nil && a.failOn(batch, inFlight) {
5959
return nil, fmt.Errorf("fake: injected analyze error for batch %q", batch.ID)
6060
}

submitqueue/extension/conflict/fileoverlap/BUILD.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ go_test(
1919
deps = [
2020
"//submitqueue/core/changeset/fake",
2121
"//submitqueue/entity",
22-
"//submitqueue/extension/conflict",
2322
"@com_github_stretchr_testify//assert",
2423
"@com_github_stretchr_testify//require",
2524
],

0 commit comments

Comments
 (0)