diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 838a800d..dfab44b5 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -5,7 +5,6 @@ go_library( srcs = [ "controller.go", "distance_filter.go", - "getchangedtargetgraph.go", "getchangedtargets.go", "gettargetgraph.go", "metrics.go", @@ -31,7 +30,6 @@ go_library( "//tangopb", "@com_github_uber_go_tally//:tally", "@org_uber_go_fx//:fx", - "@org_uber_go_yarpc//yarpcerrors", "@org_uber_go_zap//:zap", ], ) diff --git a/controller/README.md b/controller/README.md index 30277f29..aa516ebf 100644 --- a/controller/README.md +++ b/controller/README.md @@ -10,8 +10,10 @@ response streaming. The package is intentionally thin. It owns the cross-cutting concerns that sit between the wire protocol and the rest of the system: -- **Request validation and translation.** Each RPC validates its inputs and - normalizes them into the internal call shapes used downstream. +- **Request validation and translation.** GetChangedTargets validates its + inputs and normalizes them into the internal call shapes used downstream. + GetTargetGraph receives an entity request and the resolved repository + configuration from the handler, which maps and validates the request. - **Read-through caching.** Where a request can be satisfied from previously computed artifacts, the controller fetches them from storage and streams them back without invoking the orchestrator. Cache misses fall through to diff --git a/controller/controller.go b/controller/controller.go index a74de488..e1fb8ed0 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -23,6 +23,7 @@ import ( "github.com/uber/tango/config" tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" + "github.com/uber/tango/entity" "github.com/uber/tango/observability/metrics" "github.com/uber/tango/orchestrator" pb "github.com/uber/tango/tangopb" @@ -81,9 +82,11 @@ type controller struct { appCtx context.Context } -// Controller is the service implementation the handler depends on. +// Controller is the business-logic surface the handler depends on. repo is the +// configured repository that the handler resolved for the request. type Controller interface { - pb.TangoYARPCServer + GetTargetGraph(request entity.GetTargetGraphRequest, outputConfig *pb.OutputConfig, stream pb.TangoServiceGetTargetGraphYARPCServer, repo config.RepositoryConfig) error + GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error } // NewController creates a new controller. appCtx is cancelled on process diff --git a/controller/controllermock/BUILD.bazel b/controller/controllermock/BUILD.bazel index e7915924..27a0ca9b 100644 --- a/controller/controllermock/BUILD.bazel +++ b/controller/controllermock/BUILD.bazel @@ -6,6 +6,8 @@ go_library( importpath = "github.com/uber/tango/controller/controllermock", visibility = ["//visibility:public"], deps = [ + "//config", + "//entity", "//tangopb", "@org_uber_go_mock//gomock", ], diff --git a/controller/controllermock/controllermock.go b/controller/controllermock/controllermock.go index dd408742..74e448e3 100644 --- a/controller/controllermock/controllermock.go +++ b/controller/controllermock/controllermock.go @@ -12,6 +12,8 @@ package mock_controller import ( reflect "reflect" + config "github.com/uber/tango/config" + entity "github.com/uber/tango/entity" tangopb "github.com/uber/tango/tangopb" gomock "go.uber.org/mock/gomock" ) @@ -40,44 +42,30 @@ func (m *MockController) EXPECT() *MockControllerMockRecorder { return m.recorder } -// GetChangedTargetGraph mocks base method. -func (m *MockController) GetChangedTargetGraph(arg0 *tangopb.GetChangedTargetGraphRequest, arg1 tangopb.TangoServiceGetChangedTargetGraphYARPCServer) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetChangedTargetGraph", arg0, arg1) - ret0, _ := ret[0].(error) - return ret0 -} - -// GetChangedTargetGraph indicates an expected call of GetChangedTargetGraph. -func (mr *MockControllerMockRecorder) GetChangedTargetGraph(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChangedTargetGraph", reflect.TypeOf((*MockController)(nil).GetChangedTargetGraph), arg0, arg1) -} - // GetChangedTargets mocks base method. -func (m *MockController) GetChangedTargets(arg0 *tangopb.GetChangedTargetsRequest, arg1 tangopb.TangoServiceGetChangedTargetsYARPCServer) error { +func (m *MockController) GetChangedTargets(request *tangopb.GetChangedTargetsRequest, stream tangopb.TangoServiceGetChangedTargetsYARPCServer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetChangedTargets", arg0, arg1) + ret := m.ctrl.Call(m, "GetChangedTargets", request, stream) ret0, _ := ret[0].(error) return ret0 } // GetChangedTargets indicates an expected call of GetChangedTargets. -func (mr *MockControllerMockRecorder) GetChangedTargets(arg0, arg1 any) *gomock.Call { +func (mr *MockControllerMockRecorder) GetChangedTargets(request, stream any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChangedTargets", reflect.TypeOf((*MockController)(nil).GetChangedTargets), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChangedTargets", reflect.TypeOf((*MockController)(nil).GetChangedTargets), request, stream) } // GetTargetGraph mocks base method. -func (m *MockController) GetTargetGraph(arg0 *tangopb.GetTargetGraphRequest, arg1 tangopb.TangoServiceGetTargetGraphYARPCServer) error { +func (m *MockController) GetTargetGraph(request entity.GetTargetGraphRequest, outputConfig *tangopb.OutputConfig, stream tangopb.TangoServiceGetTargetGraphYARPCServer, repo config.RepositoryConfig) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetTargetGraph", arg0, arg1) + ret := m.ctrl.Call(m, "GetTargetGraph", request, outputConfig, stream, repo) ret0, _ := ret[0].(error) return ret0 } // GetTargetGraph indicates an expected call of GetTargetGraph. -func (mr *MockControllerMockRecorder) GetTargetGraph(arg0, arg1 any) *gomock.Call { +func (mr *MockControllerMockRecorder) GetTargetGraph(request, outputConfig, stream, repo any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTargetGraph", reflect.TypeOf((*MockController)(nil).GetTargetGraph), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTargetGraph", reflect.TypeOf((*MockController)(nil).GetTargetGraph), request, outputConfig, stream, repo) } diff --git a/controller/getchangedtargetgraph.go b/controller/getchangedtargetgraph.go deleted file mode 100644 index e7eb6575..00000000 --- a/controller/getchangedtargetgraph.go +++ /dev/null @@ -1,31 +0,0 @@ -// 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 controller - -import ( - tangoerrors "github.com/uber/tango/core/errors" - pb "github.com/uber/tango/tangopb" - "go.uber.org/yarpc/yarpcerrors" -) - -// GetChangedTargetGraph is the streaming RPC that will return the subgraph -// induced by the changed targets between two revisions. NOT YET IMPLEMENTED: -// returns a YARPC Unimplemented error so callers get an explicit failure -// instead of a silent empty stream. -func (c *controller) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) error { - return tangoerrors.NewInfra( - yarpcerrors.Newf(yarpcerrors.CodeUnimplemented, "GetChangedTargetGraph is not yet implemented"), - ) -} diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index b6b23c05..5cd7bb93 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -33,23 +33,16 @@ import ( "go.uber.org/zap" ) -// GetTargetGraph returns the target graph for a given request. -func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) error { - entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request) - if mappingErr != nil { - mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) - } - repoCfg, repo, repositoryErr := c.resolveRequestRepository(entityReq.Build.Remote, mappingErr) - e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) +// GetTargetGraph streams the target graph for a given request. repoCfg is the +// configured repository that the caller resolved for the request. +func (c *controller) GetTargetGraph(entityReq entity.GetTargetGraphRequest, outputConfig *pb.OutputConfig, stream pb.TangoServiceGetTargetGraphYARPCServer, repoCfg config.RepositoryConfig) error { + e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repoCfg.RepositoryID}) logger := c.logger.WithLazy( - zap.String("repository", repo), + zap.String("repository", repoCfg.RepositoryID), ) start := time.Now() ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() - if repositoryErr != nil { - return repositoryErr - } graphReader, err := c.getGraph(ctx, e, entityReq, repoCfg.RepositoryID) if err != nil { return fmt.Errorf("get graph: %w", err) @@ -60,7 +53,6 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb } defer func() { _ = graphReader.Close() }() sendStart := time.Now() - outputConfig := request.GetOutputConfig() for { chunk, err := graphReader.Read() if err == io.EOF { diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 260268a2..40152a89 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -24,11 +24,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/config" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" "github.com/uber/tango/entity" orchestratormock "github.com/uber/tango/orchestrator/orchestratormock" - pb "github.com/uber/tango/tangopb" tangomock "github.com/uber/tango/tangopb/tangopbmock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -51,18 +51,15 @@ func TestGetTargetGraph_CacheMiss_NoSend(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - req := &pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: "repo:go-code", - BaseSha: "sha", - Requests: []*pb.Request{ - {Url: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, - {Url: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, + req := entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, + {URL: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, }, }, } - err := c.GetTargetGraph(req, stream) + err := c.GetTargetGraph(req, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) } @@ -78,17 +75,14 @@ func TestGetTargetGraph_StorageError_Propagates(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: storagemock, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: "repo:go-code", - BaseSha: "sha", - Requests: []*pb.Request{ - {Url: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, - {Url: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, + {URL: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, }, }, - }, stream) + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, expected, err) } @@ -106,17 +100,14 @@ func TestGetTargetGraph_DecodeError_ReturnsError(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: storagemock, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: "repo:go-code", - BaseSha: "sha", - Requests: []*pb.Request{ - {Url: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, - {Url: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, + {URL: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, }, }, - }, stream) + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, err) } @@ -138,57 +129,17 @@ func TestGetTargetGraph_SendsWhenItemPresent(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: "repo:go-code", - BaseSha: "sha", - Requests: []*pb.Request{ - {Url: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, - {Url: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", + ChangeRequests: []entity.ChangeRequest{ + {URL: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, + {URL: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, }, }, - }, stream) + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) } -func TestGetTargetGraph_BuildDescriptionMissingRequiredFields_ReturnsError(t *testing.T) { - ctrl := gomock.NewController(t) - stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) - stream.EXPECT().Context().Return(context.Background()) - store := storagemock.NewMockStorage(ctrl) - c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, - }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{ - Strategy: pb.COMPUTATION_STRATEGY_UNSET, - Remote: "repo:go-code", - Requests: []*pb.Request{ - {Url: "github://github.com/org/repo/pull/1/1111111111111111111111111111111111111111"}, - {Url: "github://github.com/org/repo/pull/2/2222222222222222222222222222222222222222"}, - }, - }, - }, stream) - assert.Error(t, err) -} - -func TestGetTargetGraph_MissingBuildDescription_ReturnsError(t *testing.T) { - ctrl := gomock.NewController(t) - stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) - stream.EXPECT().Context().Return(context.Background()) - store := storagemock.NewMockStorage(ctrl) - c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, - }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{}, stream) - assert.Error(t, err) -} - // New coverage: Storage returns NotFound on treehash path -> orchestrator is called to compute the target graph. func TestGetTargetGraph_TreehashNotFound_NoError(t *testing.T) { ctrl := gomock.NewController(t) @@ -207,9 +158,9 @@ func TestGetTargetGraph_TreehashNotFound_NoError(t *testing.T) { Storage: store, Orchestrator: orchestrator, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) } @@ -225,9 +176,9 @@ func TestGetTargetGraph_TreehashReadError(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, err) } @@ -246,9 +197,9 @@ func TestGetTargetGraph_GraphFetchError(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.Error(t, err) } @@ -267,9 +218,9 @@ func TestGetTargetGraph_GraphReadError(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, err) } @@ -293,9 +244,9 @@ func TestGetTargetGraph_StreamSendError(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: storagemock, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, err) } @@ -319,9 +270,9 @@ func TestGetTargetGraph_GraphNotFound_FallsThrough(t *testing.T) { Storage: store, Orchestrator: orch, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) } @@ -341,9 +292,9 @@ func TestGetTargetGraph_GraphReadCancelled(t *testing.T) { Logger: zaptest.NewLogger(t), Storage: store, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.Error(t, err) } @@ -363,9 +314,9 @@ func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { Storage: store, Orchestrator: orch, }) - err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ - BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, - }, stream) + err := c.GetTargetGraph(entity.GetTargetGraphRequest{ + Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, + }, nil, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.Error(t, err) } diff --git a/handler/BUILD.bazel b/handler/BUILD.bazel index e0e62bda..85defce5 100644 --- a/handler/BUILD.bazel +++ b/handler/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//tangopb", "@com_github_uber_go_tally//:tally", "@org_uber_go_fx//:fx", + "@org_uber_go_yarpc//yarpcerrors", "@org_uber_go_zap//:zap", ], ) @@ -45,6 +46,7 @@ go_test( "//core/errors", "//core/storage", "//core/storage/storagemock", + "//entity", "//tangopb", "//tangopb/tangopbmock", "@com_github_stretchr_testify//assert", diff --git a/handler/getchangedtargetgraph.go b/handler/getchangedtargetgraph.go index 9b06b0a6..87329e6b 100644 --- a/handler/getchangedtargetgraph.go +++ b/handler/getchangedtargetgraph.go @@ -15,14 +15,23 @@ package handler import ( + tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" + "go.uber.org/yarpc/yarpcerrors" ) +// GetChangedTargetGraph is the streaming RPC that will return the subgraph +// induced by the changed targets between two revisions. NOT YET IMPLEMENTED: +// returns a YARPC Unimplemented error so callers get an explicit failure +// instead of a silent empty stream. func (h *handler) GetChangedTargetGraph(request *pb.GetChangedTargetGraphRequest, stream pb.TangoServiceGetChangedTargetGraphYARPCServer) (retErr error) { op := metrics.Begin(h.emitter, opGetChangedTargetGraph, metrics.SlowDurationBuckets) + retErr = tangoerrors.NewInfra( + yarpcerrors.Newf(yarpcerrors.CodeUnimplemented, "GetChangedTargetGraph is not yet implemented"), + ) defer func() { op.Complete(retErr) }() - return h.controller.GetChangedTargetGraph(request, stream) + return retErr } diff --git a/handler/getchangedtargetgraph_test.go b/handler/getchangedtargetgraph_test.go index 90c4e0e0..5dd9f684 100644 --- a/handler/getchangedtargetgraph_test.go +++ b/handler/getchangedtargetgraph_test.go @@ -19,27 +19,18 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - mock_controller "github.com/uber/tango/controller/controllermock" + tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" - "go.uber.org/mock/gomock" + "go.uber.org/yarpc/yarpcerrors" + "go.uber.org/zap" ) -type fakeGetChangedTargetGraphStream struct { - pb.TangoServiceGetChangedTargetGraphYARPCServer -} - -func TestGetChangedTargetGraphForwardsToController(t *testing.T) { - ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Controller: ctrl}) +func TestGetChangedTargetGraph_ReturnsUnimplemented(t *testing.T) { + h := New(Params{Logger: zap.NewNop()}) - req := &pb.GetChangedTargetGraphRequest{} - stream := &fakeGetChangedTargetGraphStream{} - ctrl.EXPECT().GetChangedTargetGraph(gomock.Any(), gomock.Any()).DoAndReturn( - func(gotReq *pb.GetChangedTargetGraphRequest, gotStream pb.TangoServiceGetChangedTargetGraphYARPCServer) error { - assert.Same(t, req, gotReq) - assert.Same(t, stream, gotStream) - return nil - }) + err := h.GetChangedTargetGraph(&pb.GetChangedTargetGraphRequest{}, nil) - require.NoError(t, h.GetChangedTargetGraph(req, stream)) + require.Error(t, err) + assert.Equal(t, yarpcerrors.CodeUnimplemented, yarpcerrors.FromError(err).Code()) + assert.Equal(t, tangoerrors.ErrorInfra, tangoerrors.GetErrorCode(err)) } diff --git a/handler/gettargetgraph.go b/handler/gettargetgraph.go index 7aa48b3f..8479f92d 100644 --- a/handler/gettargetgraph.go +++ b/handler/gettargetgraph.go @@ -25,13 +25,11 @@ import ( ) func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { - // Temporary: the handler maps the request only to tag the metrics. The - // controller maps it again until it takes the entity request. entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request) if mappingErr != nil { mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr)) } - _, repo, repositoryErr := h.resolveRequestRepository(entityReq.Build.Remote, mappingErr) + repoCfg, repo, repositoryErr := h.resolveRequestRepository(entityReq.Build.Remote, mappingErr) e := h.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) op := metrics.Begin(e, opGetTargetGraph, metrics.SlowDurationBuckets) logger := h.logger.WithLazy(zap.String("repository", repo)) @@ -45,5 +43,5 @@ func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.Ta if repositoryErr != nil { return repositoryErr } - return h.controller.GetTargetGraph(request, stream) + return h.controller.GetTargetGraph(entityReq, request.GetOutputConfig(), stream, repoCfg) } diff --git a/handler/gettargetgraph_test.go b/handler/gettargetgraph_test.go index 0bb3e1b3..0b8c2eaa 100644 --- a/handler/gettargetgraph_test.go +++ b/handler/gettargetgraph_test.go @@ -19,7 +19,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/config" mock_controller "github.com/uber/tango/controller/controllermock" + "github.com/uber/tango/entity" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" @@ -33,12 +35,17 @@ func TestGetTargetGraphForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) h := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: ctrl}) - req := &pb.GetTargetGraphRequest{BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}} + req := &pb.GetTargetGraphRequest{ + BuildDescription: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha"}, + OutputConfig: &pb.OutputConfig{IncludeHashes: true}, + } stream := &fakeGetTargetGraphStream{} - ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).DoAndReturn( - func(gotReq *pb.GetTargetGraphRequest, gotStream pb.TangoServiceGetTargetGraphYARPCServer) error { - assert.Same(t, req, gotReq) + ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(gotReq entity.GetTargetGraphRequest, gotOutput *pb.OutputConfig, gotStream pb.TangoServiceGetTargetGraphYARPCServer, gotRepo config.RepositoryConfig) error { + assert.Equal(t, "sha", gotReq.Build.BaseSha) + assert.Same(t, req.OutputConfig, gotOutput) assert.Same(t, stream, gotStream) + assert.Equal(t, "test-repository", gotRepo.RepositoryID) return nil }) diff --git a/handler/metrics_test.go b/handler/metrics_test.go index d0e915de..921638ff 100644 --- a/handler/metrics_test.go +++ b/handler/metrics_test.go @@ -15,14 +15,12 @@ package handler import ( - "errors" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally" mock_controller "github.com/uber/tango/controller/controllermock" - tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -31,7 +29,7 @@ import ( func TestGetTargetGraphMetrics(t *testing.T) { ts := tally.NewTestScope("", nil) ctrl := mock_controller.NewMockController(gomock.NewController(t)) - ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(nil) + ctrl.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) h := New(Params{Logger: zap.NewNop(), Controller: ctrl, RepoConfig: allowAnyRepositoryConfigProvider{}, Scope: ts}) build := &pb.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", Strategy: pb.COMPUTATION_STRATEGY_UNSET} @@ -58,9 +56,7 @@ func TestGetChangedTargetsMetrics(t *testing.T) { func TestGetChangedTargetGraphMetrics(t *testing.T) { ts := tally.NewTestScope("", nil) - ctrl := mock_controller.NewMockController(gomock.NewController(t)) - ctrl.EXPECT().GetChangedTargetGraph(gomock.Any(), gomock.Any()).Return(tangoerrors.NewInfra(errors.New("unimplemented"))) - h := New(Params{Logger: zap.NewNop(), Controller: ctrl, Scope: ts}) + h := New(Params{Logger: zap.NewNop(), Scope: ts}) require.Error(t, h.GetChangedTargetGraph(nil, nil))