From c34b7468143b0e0610508f16200f660b7c572114 Mon Sep 17 00:00:00 2001 From: Justin Won Date: Thu, 15 Oct 2026 02:01:51 -0700 Subject: [PATCH] refactor(controller): GetTargetGraph takes the entity request and the repository The handler already maps the GetTargetGraph request and resolves the repository. It now passes the entity request and the resolved config.RepositoryConfig to the controller, together with the output config and the response stream. The controller no longer maps the request or resolves the repository for this RPC. It uses the repository ID for its cache keys and its phase metrics. It still filters and sends to the stream. The handler no longer carries the comment that names its mapping as temporary for this RPC. The Controller interface no longer embeds the generated server, because its first method now takes entity types. It lists GetTargetGraph and GetChangedTargets. The stub GetChangedTargetGraph leaves the controller. The handler serves the stub itself: it starts the metrics.Op, returns the Unimplemented infra error, and completes the op in the deferred block, without a controller call. The handler test of the stub no longer needs a controller mock. The mock is regenerated. The controller keeps its resolver for GetChangedTargets only. Test plan: - make build, make test, make lint - handler tests check the forwarded entity request, output config, stream, and repository - controller tests call GetTargetGraph with the entity request Co-Authored-By: Claude Sonnet 5.5 --- controller/BUILD.bazel | 2 - controller/README.md | 6 +- controller/controller.go | 7 +- controller/controllermock/BUILD.bazel | 2 + controller/controllermock/controllermock.go | 32 ++--- controller/getchangedtargetgraph.go | 31 ----- controller/gettargetgraph.go | 18 +-- controller/gettargetgraph_test.go | 147 +++++++------------- handler/BUILD.bazel | 2 + handler/getchangedtargetgraph.go | 11 +- handler/getchangedtargetgraph_test.go | 27 ++-- handler/gettargetgraph.go | 6 +- handler/gettargetgraph_test.go | 15 +- handler/metrics_test.go | 8 +- 14 files changed, 111 insertions(+), 203 deletions(-) delete mode 100644 controller/getchangedtargetgraph.go 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))