diff --git a/service/submitqueue/gateway/server/BUILD.bazel b/service/submitqueue/gateway/server/BUILD.bazel index fd7b9f49..9d24713c 100644 --- a/service/submitqueue/gateway/server/BUILD.bazel +++ b/service/submitqueue/gateway/server/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "//platform/extension/counter/mysql:go_default_library", "//platform/extension/messagequeue:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", + "//service/submitqueue/gateway/server/mapper:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/extension/queueconfig/yaml:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", diff --git a/service/submitqueue/gateway/server/main.go b/service/submitqueue/gateway/server/main.go index e3f2593d..87844828 100644 --- a/service/submitqueue/gateway/server/main.go +++ b/service/submitqueue/gateway/server/main.go @@ -36,6 +36,7 @@ import ( mysqlcounter "github.com/uber/submitqueue/platform/extension/counter/mysql" extqueue "github.com/uber/submitqueue/platform/extension/messagequeue" queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" + "github.com/uber/submitqueue/service/submitqueue/gateway/server/mapper" "github.com/uber/submitqueue/submitqueue/core/topickey" yamlqueueconfig "github.com/uber/submitqueue/submitqueue/extension/queueconfig/yaml" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" @@ -62,9 +63,18 @@ func (s *GatewayServer) Ping(ctx context.Context, req *pb.PingRequest) (*pb.Ping return s.pingController.Ping(ctx, req) } -// Land delegates to the controller +// Land maps the wire request to an entity, delegates to the controller, and maps +// the result back to the wire response. func (s *GatewayServer) Land(ctx context.Context, req *pb.LandRequest) (*pb.LandResponse, error) { - return s.landController.Land(ctx, req) + landReq, err := mapper.ProtoToLandRequest(req) + if err != nil { + return nil, err + } + result, err := s.landController.Land(ctx, landReq) + if err != nil { + return nil, err + } + return &pb.LandResponse{Sqid: result.ID}, nil } // Cancel delegates to the controller diff --git a/service/submitqueue/gateway/server/mapper/BUILD.bazel b/service/submitqueue/gateway/server/mapper/BUILD.bazel new file mode 100644 index 00000000..b5179221 --- /dev/null +++ b/service/submitqueue/gateway/server/mapper/BUILD.bazel @@ -0,0 +1,31 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["land.go"], + importpath = "github.com/uber/submitqueue/service/submitqueue/gateway/server/mapper", + visibility = ["//visibility:public"], + deps = [ + "//api/base/mergestrategy/protopb:go_default_library", + "//api/submitqueue/gateway/protopb:go_default_library", + "//platform/base/change:go_default_library", + "//platform/base/mergestrategy:go_default_library", + "//submitqueue/entity:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["land_test.go"], + embed = [":go_default_library"], + deps = [ + "//api/base/change/protopb:go_default_library", + "//api/base/mergestrategy/protopb:go_default_library", + "//api/submitqueue/gateway/protopb:go_default_library", + "//platform/base/change:go_default_library", + "//platform/base/mergestrategy:go_default_library", + "//submitqueue/entity:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/service/submitqueue/gateway/server/mapper/land.go b/service/submitqueue/gateway/server/mapper/land.go new file mode 100644 index 00000000..ea3fc0d6 --- /dev/null +++ b/service/submitqueue/gateway/server/mapper/land.go @@ -0,0 +1,65 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package mapper translates gateway wire (proto) types to and from the domain +// entities the controllers operate on. Each RPC gets its own file (land.go, +// status.go, cancel.go, …); translation lives here so controllers stay +// proto-free. +package mapper + +import ( + "errors" + "fmt" + + mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/base/mergestrategy" + "github.com/uber/submitqueue/submitqueue/entity" +) + +// errUnknownStrategy is returned when a proto Strategy enum has no known +// mergestrategy.MergeStrategy mapping. +var errUnknownStrategy = errors.New("unknown land strategy in proto message") + +// ProtoToLandRequest maps the wire LandRequest to the entity.LandRequest the controller operates on. +// The ID is left empty; the controller assigns it. +func ProtoToLandRequest(req *pb.LandRequest) (entity.LandRequest, error) { + strategy, err := resolveMergeStrategy(req.GetStrategy()) + if err != nil { + return entity.LandRequest{}, fmt.Errorf("failed to map land strategy: %w", err) + } + return entity.LandRequest{ + Queue: req.GetQueue(), + Change: change.Change{URIs: req.GetChange().GetUris()}, + LandStrategy: strategy, + }, nil +} + +// resolveMergeStrategy maps a proto Strategy enum to the shared mergestrategy.MergeStrategy. +func resolveMergeStrategy(s mergestrategypb.Strategy) (mergestrategy.MergeStrategy, error) { + switch s { + case mergestrategypb.Strategy_DEFAULT: + // TODO: resolve default strategy based on queue configuration + return mergestrategy.MergeStrategyRebase, nil + case mergestrategypb.Strategy_REBASE: + return mergestrategy.MergeStrategyRebase, nil + case mergestrategypb.Strategy_SQUASH_REBASE: + return mergestrategy.MergeStrategySquashRebase, nil + case mergestrategypb.Strategy_MERGE: + return mergestrategy.MergeStrategyMerge, nil + default: + return mergestrategy.MergeStrategyUnknown, fmt.Errorf("%w: %v", errUnknownStrategy, s) + } +} diff --git a/service/submitqueue/gateway/server/mapper/land_test.go b/service/submitqueue/gateway/server/mapper/land_test.go new file mode 100644 index 00000000..72cfdd01 --- /dev/null +++ b/service/submitqueue/gateway/server/mapper/land_test.go @@ -0,0 +1,111 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mapper + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + changepb "github.com/uber/submitqueue/api/base/change/protopb" + mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/base/mergestrategy" + "github.com/uber/submitqueue/submitqueue/entity" +) + +func TestProtoToLandRequest(t *testing.T) { + const uri = "github://github.example.com/uber/test-repo/pull/1/c3a4d5e6f7890123456789abcdef0123456789ab" + + tests := []struct { + name string + req *pb.LandRequest + expected entity.LandRequest + expectedErr error + }{ + { + name: "maps all fields and leaves ID empty", + req: &pb.LandRequest{ + Queue: "test-queue", + Change: &changepb.Change{Uris: []string{uri}}, + Strategy: mergestrategypb.Strategy_SQUASH_REBASE, + }, + // ID is not assigned by the mapper — the controller mints it. + expected: entity.LandRequest{ + Queue: "test-queue", + Change: change.Change{URIs: []string{uri}}, + LandStrategy: mergestrategy.MergeStrategySquashRebase, + }, + }, + { + name: "nil change yields empty URIs without erroring", + req: &pb.LandRequest{Queue: "test-queue", Change: nil}, + // The mapper does not validate; it leaves URIs empty for the controller to reject. + expected: entity.LandRequest{ + Queue: "test-queue", + LandStrategy: mergestrategy.MergeStrategyRebase, + }, + }, + { + name: "unknown strategy errors", + req: &pb.LandRequest{ + Queue: "test-queue", + Change: &changepb.Change{Uris: []string{uri}}, + Strategy: mergestrategypb.Strategy(9999), + }, + expectedErr: errUnknownStrategy, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ProtoToLandRequest(tt.req) + if tt.expectedErr != nil { + require.ErrorIs(t, err, tt.expectedErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +func TestResolveMergeStrategy(t *testing.T) { + tests := []struct { + name string + in mergestrategypb.Strategy + want mergestrategy.MergeStrategy + errMsg string + }{ + {name: "default", in: mergestrategypb.Strategy_DEFAULT, want: mergestrategy.MergeStrategyRebase}, + {name: "rebase", in: mergestrategypb.Strategy_REBASE, want: mergestrategy.MergeStrategyRebase}, + {name: "squash_rebase", in: mergestrategypb.Strategy_SQUASH_REBASE, want: mergestrategy.MergeStrategySquashRebase}, + {name: "merge", in: mergestrategypb.Strategy_MERGE, want: mergestrategy.MergeStrategyMerge}, + {name: "unknown", in: mergestrategypb.Strategy(9999), errMsg: "unknown land strategy in proto message"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveMergeStrategy(tt.in) + if tt.errMsg != "" { + assert.ErrorContains(t, err, tt.errMsg) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/submitqueue/entity/BUILD.bazel b/submitqueue/entity/BUILD.bazel index 60d9fa82..21a945e0 100644 --- a/submitqueue/entity/BUILD.bazel +++ b/submitqueue/entity/BUILD.bazel @@ -11,7 +11,7 @@ go_library( "change_provider.go", "change_record.go", "conflict.go", - "land_request.go", + "land.go", "merge_result.go", "push_result.go", "queue.go", @@ -36,7 +36,7 @@ go_test( "batch_test.go", "build_test.go", "cancel_request_test.go", - "land_request_test.go", + "land_test.go", "queue_test.go", "request_log_test.go", "request_test.go", diff --git a/submitqueue/entity/land_request.go b/submitqueue/entity/land.go similarity index 84% rename from submitqueue/entity/land_request.go rename to submitqueue/entity/land.go index a9b7a609..e0c96e17 100644 --- a/submitqueue/entity/land_request.go +++ b/submitqueue/entity/land.go @@ -46,3 +46,12 @@ func LandRequestFromBytes(data []byte) (LandRequest, error) { err := json.Unmarshal(data, &req) return req, err } + +// LandResult is the outcome of accepting a land request. It carries the ID the +// controller assigned to the request so the transport layer can echo it back to +// the caller. +type LandResult struct { + // ID is the globally unique identifier assigned to the accepted land request. + // Format: "/". + ID string +} diff --git a/submitqueue/entity/land_request_test.go b/submitqueue/entity/land_test.go similarity index 100% rename from submitqueue/entity/land_request_test.go rename to submitqueue/entity/land_test.go diff --git a/submitqueue/gateway/controller/BUILD.bazel b/submitqueue/gateway/controller/BUILD.bazel index 45a7c6c9..eac0b0b5 100644 --- a/submitqueue/gateway/controller/BUILD.bazel +++ b/submitqueue/gateway/controller/BUILD.bazel @@ -11,10 +11,7 @@ go_library( importpath = "github.com/uber/submitqueue/submitqueue/gateway/controller", visibility = ["//visibility:public"], deps = [ - "//api/base/mergestrategy/protopb:go_default_library", "//api/submitqueue/gateway/protopb:go_default_library", - "//platform/base/change:go_default_library", - "//platform/base/mergestrategy:go_default_library", "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/errs:go_default_library", @@ -40,9 +37,8 @@ go_test( ], embed = [":go_default_library"], deps = [ - "//api/base/change/protopb:go_default_library", - "//api/base/mergestrategy/protopb:go_default_library", "//api/submitqueue/gateway/protopb:go_default_library", + "//platform/base/change:go_default_library", "//platform/base/mergestrategy:go_default_library", "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", diff --git a/submitqueue/gateway/controller/land.go b/submitqueue/gateway/controller/land.go index 6e15c72f..c44dd84f 100644 --- a/submitqueue/gateway/controller/land.go +++ b/submitqueue/gateway/controller/land.go @@ -20,10 +20,6 @@ import ( "fmt" "github.com/uber-go/tally" - mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" - pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" - "github.com/uber/submitqueue/platform/base/change" - "github.com/uber/submitqueue/platform/base/mergestrategy" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/errs" @@ -87,8 +83,8 @@ func NewLandController(logger *zap.SugaredLogger, scope tally.Scope, counter cou } } -// Land handles the land request and returns a response -func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (resp *pb.LandResponse, retErr error) { +// Land handles the land request and returns the ID assigned to the accepted request. +func (c *LandController) Land(ctx context.Context, req entity.LandRequest) (result entity.LandResult, retErr error) { const opName = "land" op := metrics.Begin(c.metricsScope, opName) @@ -96,74 +92,57 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (resp *p // Validate required fields. if req.Queue == "" { - return nil, fmt.Errorf("LandController requires the request to have a queue name specified: %w", ErrInvalidRequest) + return entity.LandResult{}, fmt.Errorf("LandController requires the request to have a queue name specified: %w", ErrInvalidRequest) } - if req.Change == nil || len(req.Change.Uris) == 0 { - return nil, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest) - } - - change := change.Change{ - URIs: req.Change.GetUris(), + if len(req.Change.URIs) == 0 { + return entity.LandResult{}, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest) } queue := req.Queue if _, err := c.queueConfigs.Get(ctx, queue); err != nil { if errors.Is(err, queueconfig.ErrNotFound) { - return nil, errs.NewUserError(&UnrecognizedQueueError{Queue: queue}) + return entity.LandResult{}, errs.NewUserError(&UnrecognizedQueueError{Queue: queue}) } - return nil, fmt.Errorf("LandController failed to look up queue %q: %w", queue, err) - } - - // TODO: pass default queue land strategy to resolver function to process a default. - strategy, err := resolveMergeStrategy(req.Strategy) - if err != nil { - return nil, fmt.Errorf("LandController failed to map strategy for queue=%s: %w", req.Queue, err) + return entity.LandResult{}, fmt.Errorf("LandController failed to look up queue %q: %w", queue, err) } // Generate a globally unique request ID for the land request. + // The inbound entity arrives with an empty ID; the controller owns minting it. seq, err := c.counter.Next(ctx, "request/"+queue) if err != nil { - return nil, fmt.Errorf("LandController failed to generate request ID for queue=%s: %w", queue, err) - } - - landRequest := entity.LandRequest{ - ID: fmt.Sprintf("%s/%d", queue, seq), - Queue: queue, - Change: change, - LandStrategy: strategy, + return entity.LandResult{}, fmt.Errorf("LandController failed to generate request ID for queue=%s: %w", queue, err) } + req.ID = fmt.Sprintf("%s/%d", queue, seq) // Record the accepted status in the request log for reconciliation. Once the request materializes as a Request entity, the status might be updated to "new". // It is important to record the status before publishing to the queue for processing. It is important to publish straight to the database and not via a entityqueue. // Gateway has to stay consistent with the request log. - logEntry := entity.NewRequestLog(landRequest.ID, entity.RequestStatusAccepted, 0, "", nil) + logEntry := entity.NewRequestLog(req.ID, entity.RequestStatusAccepted, 0, "", nil) if err := c.store.GetRequestLogStore().Insert(ctx, logEntry); err != nil { - return nil, fmt.Errorf("LandController failed to insert request log for sqid=%s: %w", landRequest.ID, err) + return entity.LandResult{}, fmt.Errorf("LandController failed to insert request log for sqid=%s: %w", req.ID, err) } c.logger.Debugw("land request created", "queue", req.Queue, - "sqid", landRequest.ID, - "change_uris", change.URIs, - "change_count", len(change.URIs), - "strategy", string(strategy), + "sqid", req.ID, + "change_uris", req.Change.URIs, + "change_count", len(req.Change.URIs), + "strategy", string(req.LandStrategy), ) // Publish to queue for async processing - if err := c.publishToQueue(ctx, landRequest); err != nil { - return nil, fmt.Errorf("LandController failed to publish request to queue: %w", err) + if err := c.publishToQueue(ctx, req); err != nil { + return entity.LandResult{}, fmt.Errorf("LandController failed to publish request to queue: %w", err) } c.logger.Infow("request published to queue", "queue", req.Queue, - "sqid", landRequest.ID, + "sqid", req.ID, "topic_key", topickey.TopicKeyStart, ) metrics.NamedCounter(c.metricsScope, opName, "publish_success", 1) - return &pb.LandResponse{ - Sqid: landRequest.ID, - }, nil + return entity.LandResult{ID: req.ID}, nil } // publishToQueue publishes a land request to the request queue for async processing. @@ -196,20 +175,3 @@ func (c *LandController) publishToQueue(ctx context.Context, landRequest entity. return nil } - -// resolveMergeStrategy maps a proto Strategy enum to the shared mergestrategy.MergeStrategy. -func resolveMergeStrategy(s mergestrategypb.Strategy) (mergestrategy.MergeStrategy, error) { - switch s { - case mergestrategypb.Strategy_DEFAULT: - // TODO: resolve default strategy based on queue configuration - return mergestrategy.MergeStrategyRebase, nil - case mergestrategypb.Strategy_REBASE: - return mergestrategy.MergeStrategyRebase, nil - case mergestrategypb.Strategy_SQUASH_REBASE: - return mergestrategy.MergeStrategySquashRebase, nil - case mergestrategypb.Strategy_MERGE: - return mergestrategy.MergeStrategyMerge, nil - default: - return mergestrategy.MergeStrategyUnknown, fmt.Errorf("unknown land strategy in proto message: %v", s) - } -} diff --git a/submitqueue/gateway/controller/land_test.go b/submitqueue/gateway/controller/land_test.go index 63df3c11..9f004d0f 100644 --- a/submitqueue/gateway/controller/land_test.go +++ b/submitqueue/gateway/controller/land_test.go @@ -22,9 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally" - changepb "github.com/uber/submitqueue/api/base/change/protopb" - mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" - pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/platform/base/mergestrategy" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" @@ -84,6 +82,16 @@ func noopQueueConfigStore(ctrl *gomock.Controller) *qcmock.MockStore { return s } +// testLandRequest returns a valid entity.LandRequest for the given queue. The ID +// is intentionally left empty — the controller assigns it. +func testLandRequest(queue string) entity.LandRequest { + return entity.LandRequest{ + Queue: queue, + Change: change.Change{URIs: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, + LandStrategy: mergestrategy.MergeStrategyRebase, + } +} + func TestNewLandController(t *testing.T) { ctrl := gomock.NewController(t) @@ -100,14 +108,10 @@ func TestLand_ReturnsSqid(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - resp, err := controller.Land(ctx, req) + result, err := controller.Land(ctx, testLandRequest("test-queue")) require.NoError(t, err) - assert.Equal(t, "test-queue/1", resp.Sqid) + assert.Equal(t, "test-queue/1", result.ID) } func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { @@ -118,11 +122,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - _, err := controller.Land(ctx, req) + _, err := controller.Land(ctx, testLandRequest("test-queue")) require.Error(t, err) } @@ -142,11 +142,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "my-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - _, err := controller.Land(ctx, req) + _, err := controller.Land(ctx, testLandRequest("my-queue")) require.NoError(t, err) assert.Equal(t, "request/my-queue", capturedDomain) @@ -159,10 +155,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } + req := testLandRequest("") _, err := controller.Land(ctx, req) require.Error(t, err) @@ -176,9 +169,9 @@ func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ + req := entity.LandRequest{ Queue: "test-queue", - Change: &changepb.Change{Uris: []string{}}, + Change: change.Change{URIs: []string{}}, } _, err := controller.Land(ctx, req) @@ -186,16 +179,16 @@ func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) { assert.True(t, IsInvalidRequest(err)) } -func TestLand_ReturnsErrorOnNilChange(t *testing.T) { +func TestLand_ReturnsErrorOnZeroValueChange(t *testing.T) { ctrl := gomock.NewController(t) cnt := countermock.NewMockCounter(ctrl) controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ + req := entity.LandRequest{ Queue: "test-queue", - Change: nil, + Change: change.Change{}, } _, err := controller.Land(ctx, req) @@ -213,11 +206,7 @@ func TestLand_ReturnsUnrecognizedQueueWhenStoreReportsNotFound(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "missing-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - _, err := controller.Land(ctx, req) + _, err := controller.Land(ctx, testLandRequest("missing-queue")) require.Error(t, err) assert.True(t, IsUnrecognizedQueue(err)) @@ -239,11 +228,7 @@ func TestLand_PropagatesQueueConfigStoreError(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), qcs, newTestRegistryWithNoopPublisher(t, ctrl)) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - _, err := controller.Land(ctx, req) + _, err := controller.Land(ctx, testLandRequest("test-queue")) require.Error(t, err) assert.False(t, IsUnrecognizedQueue(err)) @@ -271,15 +256,15 @@ func TestLand_PublishesToQueue(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), registry) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98"}}, - Strategy: mergestrategypb.Strategy_REBASE, + req := entity.LandRequest{ + Queue: "test-queue", + Change: change.Change{URIs: []string{"github://github.example.com/uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98"}}, + LandStrategy: mergestrategy.MergeStrategyRebase, } - resp, err := controller.Land(ctx, req) + result, err := controller.Land(ctx, req) require.NoError(t, err) - assert.Equal(t, "test-queue/123", resp.Sqid) + assert.Equal(t, "test-queue/123", result.ID) // Verify message was published to the topic registered under TopicKeyStart assert.Equal(t, "start", publishedTopic) @@ -307,11 +292,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, cnt, noopStorage(ctrl), noopQueueConfigStore(ctrl), registry) ctx := context.Background() - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &changepb.Change{Uris: []string{"github://github.example.com/uber/service/pull/1/c3a4d5e6f7890123456789abcdef0123456789ab"}}, - } - _, err := controller.Land(ctx, req) + _, err := controller.Land(ctx, testLandRequest("test-queue")) // Should fail if publish fails require.Error(t, err)