From 9fca840a1693ba80c7e66fe20f3147553c557eb0 Mon Sep 17 00:00:00 2001 From: "kevin.new" Date: Thu, 26 Feb 2026 22:17:29 +0000 Subject: [PATCH 1/4] feat(landprovider): Adding land provider extension interface --- extension/landprovider/BUILD.bazel | 9 +++ extension/landprovider/land_provider.go | 32 +++++++++++ extension/landprovider/mock/BUILD.bazel | 12 ++++ .../landprovider/mock/land_provider_mock.go | 57 +++++++++++++++++++ 4 files changed, 110 insertions(+) create mode 100644 extension/landprovider/BUILD.bazel create mode 100644 extension/landprovider/land_provider.go create mode 100644 extension/landprovider/mock/BUILD.bazel create mode 100644 extension/landprovider/mock/land_provider_mock.go diff --git a/extension/landprovider/BUILD.bazel b/extension/landprovider/BUILD.bazel new file mode 100644 index 00000000..4614b004 --- /dev/null +++ b/extension/landprovider/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "landprovider", + srcs = ["land_provider.go"], + importpath = "github.com/uber/submitqueue/extension/landprovider", + visibility = ["//visibility:public"], + deps = ["//entity"], +) diff --git a/extension/landprovider/land_provider.go b/extension/landprovider/land_provider.go new file mode 100644 index 00000000..29589620 --- /dev/null +++ b/extension/landprovider/land_provider.go @@ -0,0 +1,32 @@ +package landprovider + +import ( + "context" + + "github.com/uber/submitqueue/entity" +) + +// LandEntry pairs a land strategy with the change to land. +// Each entry represents one request's contribution to a batch land operation. +type LandEntry struct { + // Strategy is the source control integration method for this change. + Strategy entity.RequestLandStrategy + // Change is the code change to land. + Change entity.Change +} + +// LandProvider lands (merges) code changes into the target branch of a source +// control repository. Each implementation is configured for a specific provider +// (GitHub, GitLab, Phabricator). +type LandProvider interface { + // Land merges the provided changes into the target branch of the given queue. + // The queue identifies the repository and target branch. + // Each entry contains a change and the strategy to use for landing it. + Land(ctx context.Context, queue string, entries []LandEntry) (Result, error) +} + +// Result holds the outcome of a land operation. +type Result struct { + // SHA is the commit SHA of the landed change on the target branch. + SHA string +} diff --git a/extension/landprovider/mock/BUILD.bazel b/extension/landprovider/mock/BUILD.bazel new file mode 100644 index 00000000..4ea68795 --- /dev/null +++ b/extension/landprovider/mock/BUILD.bazel @@ -0,0 +1,12 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "mock", + srcs = ["land_provider_mock.go"], + importpath = "github.com/uber/submitqueue/extension/landprovider/mock", + visibility = ["//visibility:public"], + deps = [ + "//extension/landprovider", + "@org_uber_go_mock//gomock", + ], +) diff --git a/extension/landprovider/mock/land_provider_mock.go b/extension/landprovider/mock/land_provider_mock.go new file mode 100644 index 00000000..8d6afbf8 --- /dev/null +++ b/extension/landprovider/mock/land_provider_mock.go @@ -0,0 +1,57 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: extension/landprovider/land_provider.go +// +// Generated by this command: +// +// mockgen -source=extension/landprovider/land_provider.go -destination=extension/landprovider/mock/land_provider_mock.go -package=mock +// + +// Package mock is a generated GoMock package. +package mock + +import ( + context "context" + reflect "reflect" + + landprovider "github.com/uber/submitqueue/extension/landprovider" + gomock "go.uber.org/mock/gomock" +) + +// MockLandProvider is a mock of LandProvider interface. +type MockLandProvider struct { + ctrl *gomock.Controller + recorder *MockLandProviderMockRecorder + isgomock struct{} +} + +// MockLandProviderMockRecorder is the mock recorder for MockLandProvider. +type MockLandProviderMockRecorder struct { + mock *MockLandProvider +} + +// NewMockLandProvider creates a new mock instance. +func NewMockLandProvider(ctrl *gomock.Controller) *MockLandProvider { + mock := &MockLandProvider{ctrl: ctrl} + mock.recorder = &MockLandProviderMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockLandProvider) EXPECT() *MockLandProviderMockRecorder { + return m.recorder +} + +// Land mocks base method. +func (m *MockLandProvider) Land(ctx context.Context, queue string, entries []landprovider.LandEntry) (landprovider.Result, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Land", ctx, queue, entries) + ret0, _ := ret[0].(landprovider.Result) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Land indicates an expected call of Land. +func (mr *MockLandProviderMockRecorder) Land(ctx, queue, entries any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Land", reflect.TypeOf((*MockLandProvider)(nil).Land), ctx, queue, entries) +} From c78ac9949b324c7186647aecc90c977265f887e5 Mon Sep 17 00:00:00 2001 From: "kevin.new" Date: Fri, 27 Feb 2026 19:38:56 +0000 Subject: [PATCH 2/4] add a sentinel error for later --- extension/landprovider/BUILD.bazel | 11 +++- extension/landprovider/errors.go | 23 ++++++++ extension/landprovider/land_entry.go | 12 ++++ extension/landprovider/land_provider.go | 24 ++------ extension/landprovider/mock/BUILD.bazel | 15 ++++- .../landprovider/mock/land_provider_mock.go | 57 ------------------- 6 files changed, 63 insertions(+), 79 deletions(-) create mode 100644 extension/landprovider/errors.go create mode 100644 extension/landprovider/land_entry.go delete mode 100644 extension/landprovider/mock/land_provider_mock.go diff --git a/extension/landprovider/BUILD.bazel b/extension/landprovider/BUILD.bazel index 4614b004..4567f20a 100644 --- a/extension/landprovider/BUILD.bazel +++ b/extension/landprovider/BUILD.bazel @@ -2,8 +2,17 @@ load("@rules_go//go:def.bzl", "go_library") go_library( name = "landprovider", - srcs = ["land_provider.go"], + srcs = [ + "errors.go", + "land_entry.go", + "land_provider.go", + ], importpath = "github.com/uber/submitqueue/extension/landprovider", visibility = ["//visibility:public"], deps = ["//entity"], ) + +exports_files( + ["land_provider.go"], + visibility = ["//extension/landprovider/mock:__pkg__"], +) diff --git a/extension/landprovider/errors.go b/extension/landprovider/errors.go new file mode 100644 index 00000000..a0fc80ca --- /dev/null +++ b/extension/landprovider/errors.go @@ -0,0 +1,23 @@ +package landprovider + +import ( + "errors" + "fmt" +) + +// ErrLandRejected is returned by LandProvider implementations when the land operation +// was attempted but rejected due to the changes themselves (e.g., merge conflict, policy +// violation). This is a terminal failure — retrying will not help. +// Infrastructure errors (network timeout, API unavailable) should be returned as plain +// errors so the consumer can retry. +var ErrLandRejected = errors.New("land rejected") + +// IsLandRejected returns true if any error in the error chain is an ErrLandRejected. +func IsLandRejected(err error) bool { + return errors.Is(err, ErrLandRejected) +} + +// WrapLandRejected wraps ErrLandRejected with a descriptive reason from the land provider. +func WrapLandRejected(err error) error { + return fmt.Errorf("%w: %w", ErrLandRejected, err) +} diff --git a/extension/landprovider/land_entry.go b/extension/landprovider/land_entry.go new file mode 100644 index 00000000..5cea4174 --- /dev/null +++ b/extension/landprovider/land_entry.go @@ -0,0 +1,12 @@ +package landprovider + +import "github.com/uber/submitqueue/entity" + +// LandEntry pairs a land strategy with the change to land. +// Each entry represents one request's contribution to a batch land operation. +type LandEntry struct { + // Strategy is the source control integration method for this change. + Strategy entity.RequestLandStrategy + // Change is the code change to land. + Change entity.Change +} diff --git a/extension/landprovider/land_provider.go b/extension/landprovider/land_provider.go index 29589620..a22ad3e4 100644 --- a/extension/landprovider/land_provider.go +++ b/extension/landprovider/land_provider.go @@ -1,19 +1,8 @@ package landprovider -import ( - "context" +//go:generate mockgen -source=land_provider.go -destination=mock/land_provider_mock.go -package=mock - "github.com/uber/submitqueue/entity" -) - -// LandEntry pairs a land strategy with the change to land. -// Each entry represents one request's contribution to a batch land operation. -type LandEntry struct { - // Strategy is the source control integration method for this change. - Strategy entity.RequestLandStrategy - // Change is the code change to land. - Change entity.Change -} +import "context" // LandProvider lands (merges) code changes into the target branch of a source // control repository. Each implementation is configured for a specific provider @@ -22,11 +11,6 @@ type LandProvider interface { // Land merges the provided changes into the target branch of the given queue. // The queue identifies the repository and target branch. // Each entry contains a change and the strategy to use for landing it. - Land(ctx context.Context, queue string, entries []LandEntry) (Result, error) -} - -// Result holds the outcome of a land operation. -type Result struct { - // SHA is the commit SHA of the landed change on the target branch. - SHA string + // Returns ErrLandRejected if the land was rejected due to the changes themselves. + Land(ctx context.Context, queue string, entries []LandEntry) error } diff --git a/extension/landprovider/mock/BUILD.bazel b/extension/landprovider/mock/BUILD.bazel index 4ea68795..44e35401 100644 --- a/extension/landprovider/mock/BUILD.bazel +++ b/extension/landprovider/mock/BUILD.bazel @@ -1,8 +1,21 @@ +load("@rules_go//extras:gomock.bzl", "gomock") load("@rules_go//go:def.bzl", "go_library") +_MOCKGEN = "@org_uber_go_mock//mockgen" + +gomock( + name = "mock_land_provider_src", + out = "land_provider_mock.go", + mockgen_tool = _MOCKGEN, + package = "mock", + source = "//extension/landprovider:land_provider.go", + source_importpath = "github.com/uber/submitqueue/extension/landprovider", +) + +# gazelle:ignore go_library( name = "mock", - srcs = ["land_provider_mock.go"], + srcs = [":mock_land_provider_src"], importpath = "github.com/uber/submitqueue/extension/landprovider/mock", visibility = ["//visibility:public"], deps = [ diff --git a/extension/landprovider/mock/land_provider_mock.go b/extension/landprovider/mock/land_provider_mock.go deleted file mode 100644 index 8d6afbf8..00000000 --- a/extension/landprovider/mock/land_provider_mock.go +++ /dev/null @@ -1,57 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: extension/landprovider/land_provider.go -// -// Generated by this command: -// -// mockgen -source=extension/landprovider/land_provider.go -destination=extension/landprovider/mock/land_provider_mock.go -package=mock -// - -// Package mock is a generated GoMock package. -package mock - -import ( - context "context" - reflect "reflect" - - landprovider "github.com/uber/submitqueue/extension/landprovider" - gomock "go.uber.org/mock/gomock" -) - -// MockLandProvider is a mock of LandProvider interface. -type MockLandProvider struct { - ctrl *gomock.Controller - recorder *MockLandProviderMockRecorder - isgomock struct{} -} - -// MockLandProviderMockRecorder is the mock recorder for MockLandProvider. -type MockLandProviderMockRecorder struct { - mock *MockLandProvider -} - -// NewMockLandProvider creates a new mock instance. -func NewMockLandProvider(ctrl *gomock.Controller) *MockLandProvider { - mock := &MockLandProvider{ctrl: ctrl} - mock.recorder = &MockLandProviderMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockLandProvider) EXPECT() *MockLandProviderMockRecorder { - return m.recorder -} - -// Land mocks base method. -func (m *MockLandProvider) Land(ctx context.Context, queue string, entries []landprovider.LandEntry) (landprovider.Result, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Land", ctx, queue, entries) - ret0, _ := ret[0].(landprovider.Result) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Land indicates an expected call of Land. -func (mr *MockLandProviderMockRecorder) Land(ctx, queue, entries any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Land", reflect.TypeOf((*MockLandProvider)(nil).Land), ctx, queue, entries) -} From 72ec04e6971aa85ade4fa71243df8b2d0eaf8645 Mon Sep 17 00:00:00 2001 From: "kevin.new" Date: Mon, 2 Mar 2026 18:30:23 +0000 Subject: [PATCH 3/4] move land entry to entity --- entity/BUILD.bazel | 1 + {extension/landprovider => entity}/land_entry.go | 8 +++----- extension/landprovider/BUILD.bazel | 1 - extension/landprovider/land_provider.go | 8 ++++++-- extension/landprovider/mock/BUILD.bazel | 1 + 5 files changed, 11 insertions(+), 8 deletions(-) rename {extension/landprovider => entity}/land_entry.go (68%) diff --git a/entity/BUILD.bazel b/entity/BUILD.bazel index ccd288d0..344cc689 100644 --- a/entity/BUILD.bazel +++ b/entity/BUILD.bazel @@ -7,6 +7,7 @@ go_library( "batch_dependent.go", "build.go", "change_provider.go", + "land_entry.go", "queue_config.go", "request.go", "speculation_tree.go", diff --git a/extension/landprovider/land_entry.go b/entity/land_entry.go similarity index 68% rename from extension/landprovider/land_entry.go rename to entity/land_entry.go index 5cea4174..22b97a2a 100644 --- a/extension/landprovider/land_entry.go +++ b/entity/land_entry.go @@ -1,12 +1,10 @@ -package landprovider - -import "github.com/uber/submitqueue/entity" +package entity // LandEntry pairs a land strategy with the change to land. // Each entry represents one request's contribution to a batch land operation. type LandEntry struct { // Strategy is the source control integration method for this change. - Strategy entity.RequestLandStrategy + Strategy RequestLandStrategy // Change is the code change to land. - Change entity.Change + Change Change } diff --git a/extension/landprovider/BUILD.bazel b/extension/landprovider/BUILD.bazel index 4567f20a..34adfa64 100644 --- a/extension/landprovider/BUILD.bazel +++ b/extension/landprovider/BUILD.bazel @@ -4,7 +4,6 @@ go_library( name = "landprovider", srcs = [ "errors.go", - "land_entry.go", "land_provider.go", ], importpath = "github.com/uber/submitqueue/extension/landprovider", diff --git a/extension/landprovider/land_provider.go b/extension/landprovider/land_provider.go index a22ad3e4..3f9f9120 100644 --- a/extension/landprovider/land_provider.go +++ b/extension/landprovider/land_provider.go @@ -2,7 +2,11 @@ package landprovider //go:generate mockgen -source=land_provider.go -destination=mock/land_provider_mock.go -package=mock -import "context" +import ( + "context" + + "github.com/uber/submitqueue/entity" +) // LandProvider lands (merges) code changes into the target branch of a source // control repository. Each implementation is configured for a specific provider @@ -12,5 +16,5 @@ type LandProvider interface { // The queue identifies the repository and target branch. // Each entry contains a change and the strategy to use for landing it. // Returns ErrLandRejected if the land was rejected due to the changes themselves. - Land(ctx context.Context, queue string, entries []LandEntry) error + Land(ctx context.Context, queue string, entries []entity.LandEntry) error } diff --git a/extension/landprovider/mock/BUILD.bazel b/extension/landprovider/mock/BUILD.bazel index 44e35401..2e01efbb 100644 --- a/extension/landprovider/mock/BUILD.bazel +++ b/extension/landprovider/mock/BUILD.bazel @@ -19,6 +19,7 @@ go_library( importpath = "github.com/uber/submitqueue/extension/landprovider/mock", visibility = ["//visibility:public"], deps = [ + "//entity", "//extension/landprovider", "@org_uber_go_mock//gomock", ], From 9cee21bd50afd8a9ad8d3205587415ba3e44e159 Mon Sep 17 00:00:00 2001 From: "kevin.new" Date: Mon, 2 Mar 2026 20:40:19 +0000 Subject: [PATCH 4/4] add new sentinel 'error' for already merged for idempotence --- extension/landprovider/errors.go | 10 ++++++++++ extension/landprovider/land_provider.go | 1 + 2 files changed, 11 insertions(+) diff --git a/extension/landprovider/errors.go b/extension/landprovider/errors.go index a0fc80ca..5c7c95d0 100644 --- a/extension/landprovider/errors.go +++ b/extension/landprovider/errors.go @@ -12,11 +12,21 @@ import ( // errors so the consumer can retry. var ErrLandRejected = errors.New("land rejected") +// ErrAlreadyLanded is returned by LandProvider implementations when the changes have +// already been landed (e.g., PR already merged). The extension reports this as a domain +// fact; the controller decides whether to treat it as success. +var ErrAlreadyLanded = errors.New("already landed") + // IsLandRejected returns true if any error in the error chain is an ErrLandRejected. func IsLandRejected(err error) bool { return errors.Is(err, ErrLandRejected) } +// IsAlreadyLanded returns true if any error in the error chain is an ErrAlreadyLanded. +func IsAlreadyLanded(err error) bool { + return errors.Is(err, ErrAlreadyLanded) +} + // WrapLandRejected wraps ErrLandRejected with a descriptive reason from the land provider. func WrapLandRejected(err error) error { return fmt.Errorf("%w: %w", ErrLandRejected, err) diff --git a/extension/landprovider/land_provider.go b/extension/landprovider/land_provider.go index 3f9f9120..a83aaf2d 100644 --- a/extension/landprovider/land_provider.go +++ b/extension/landprovider/land_provider.go @@ -16,5 +16,6 @@ type LandProvider interface { // The queue identifies the repository and target branch. // Each entry contains a change and the strategy to use for landing it. // Returns ErrLandRejected if the land was rejected due to the changes themselves. + // Returns ErrAlreadyLanded if the changes have already been landed. Land(ctx context.Context, queue string, entries []entity.LandEntry) error }