diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 4e1485f6..0f3c0fee 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -5,7 +5,6 @@ go_library( srcs = [ "controller.go", "distance_filter.go", - "errors.go", "getchangedtargetgraph.go", "getchangedtargets.go", "gettargetgraph.go", @@ -48,7 +47,6 @@ go_test( "metrics_test.go", "output_filter_test.go", "testhelper_test.go", - "wireerror_test.go", ], embed = [":controller"], deps = [ @@ -66,8 +64,6 @@ go_test( "@com_github_stretchr_testify//require", "@com_github_uber_go_tally//:tally", "@org_uber_go_mock//gomock", - "@org_uber_go_yarpc//encoding/protobuf", - "@org_uber_go_yarpc//yarpcerrors", "@org_uber_go_zap//:zap", "@org_uber_go_zap//zaptest", ], diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 2dd1b5f0..941565eb 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -95,10 +95,6 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str ) defer func() { op.Complete(retErr) - if retErr != nil { - logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) - retErr = toWireError(retErr) - } }() if repositoryErr != nil { return repositoryErr diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 82e61790..621fd95f 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -47,10 +47,6 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb ) defer func() { op.Complete(retErr) - if retErr != nil { - logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) - retErr = toWireError(retErr) - } }() start := time.Now() ctx, cancelLink := c.linkRequestCtx(stream.Context()) diff --git a/example/main.go b/example/main.go index d5352278..449a7b1a 100644 --- a/example/main.go +++ b/example/main.go @@ -109,7 +109,7 @@ func run() error { GraphConfig: cfg, }) - h := handler.New(handler.Params{Controller: ctrl}) + h := handler.New(handler.Params{Logger: logger, Controller: ctrl}) // YARPC transports and dispatcher grpcTransport := yarpcgrpc.NewTransport() diff --git a/handler/BUILD.bazel b/handler/BUILD.bazel index ab00eabc..e049736e 100644 --- a/handler/BUILD.bazel +++ b/handler/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "handler", srcs = [ + "errors.go", "getchangedtargetgraph.go", "getchangedtargets.go", "gettargetgraph.go", @@ -12,7 +13,10 @@ go_library( visibility = ["//visibility:public"], deps = [ "//controller", + "//core/errors", + "//mapper/proto", "//tangopb", + "@org_uber_go_zap//:zap", ], ) @@ -22,13 +26,24 @@ go_test( "getchangedtargetgraph_test.go", "getchangedtargets_test.go", "gettargetgraph_test.go", + "testhelper_test.go", + "wireerror_test.go", ], embed = [":handler"], deps = [ + "//config", + "//controller", "//controller/controllermock", + "//core/errors", + "//core/storage", + "//core/storage/storagemock", "//tangopb", + "//tangopb/tangopbmock", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_uber_go_mock//gomock", + "@org_uber_go_yarpc//encoding/protobuf", + "@org_uber_go_yarpc//yarpcerrors", + "@org_uber_go_zap//zaptest", ], ) diff --git a/controller/errors.go b/handler/errors.go similarity index 98% rename from controller/errors.go rename to handler/errors.go index 3a3d6745..73a310b8 100644 --- a/controller/errors.go +++ b/handler/errors.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package controller +package handler import ( "github.com/uber/tango/mapper/proto" diff --git a/handler/getchangedtargets.go b/handler/getchangedtargets.go index 3d7cea3a..a7a064ad 100644 --- a/handler/getchangedtargets.go +++ b/handler/getchangedtargets.go @@ -15,9 +15,16 @@ package handler import ( + tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" ) -func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error { +func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { + defer func() { + if retErr != nil { + h.logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) + retErr = toWireError(retErr) + } + }() return h.controller.GetChangedTargets(request, stream) } diff --git a/handler/getchangedtargets_test.go b/handler/getchangedtargets_test.go index 3b841c94..f12b3811 100644 --- a/handler/getchangedtargets_test.go +++ b/handler/getchangedtargets_test.go @@ -22,6 +22,7 @@ import ( mock_controller "github.com/uber/tango/controller/controllermock" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" ) type fakeGetChangedTargetsStream struct { @@ -30,7 +31,7 @@ type fakeGetChangedTargetsStream struct { func TestGetChangedTargetsForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) req := &pb.GetChangedTargetsRequest{} stream := &fakeGetChangedTargetsStream{} diff --git a/handler/gettargetgraph.go b/handler/gettargetgraph.go index 6c1a19db..c0b1c4e4 100644 --- a/handler/gettargetgraph.go +++ b/handler/gettargetgraph.go @@ -15,9 +15,16 @@ package handler import ( + tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" ) -func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) error { +func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { + defer func() { + if retErr != nil { + h.logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) + retErr = toWireError(retErr) + } + }() return h.controller.GetTargetGraph(request, stream) } diff --git a/handler/gettargetgraph_test.go b/handler/gettargetgraph_test.go index e502aed6..58da7e74 100644 --- a/handler/gettargetgraph_test.go +++ b/handler/gettargetgraph_test.go @@ -22,6 +22,7 @@ import ( mock_controller "github.com/uber/tango/controller/controllermock" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" ) type fakeGetTargetGraphStream struct { @@ -30,7 +31,7 @@ type fakeGetTargetGraphStream struct { func TestGetTargetGraphForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) req := &pb.GetTargetGraphRequest{} stream := &fakeGetTargetGraphStream{} diff --git a/handler/handler.go b/handler/handler.go index aff260b7..2c0f5396 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -17,16 +17,19 @@ package handler import ( "github.com/uber/tango/controller" pb "github.com/uber/tango/tangopb" + "go.uber.org/zap" ) type Params struct { + Logger *zap.Logger Controller controller.Controller } type handler struct { + logger *zap.Logger controller controller.Controller } func New(p Params) pb.TangoYARPCServer { - return &handler{controller: p.Controller} + return &handler{logger: p.Logger, controller: p.Controller} } diff --git a/handler/testhelper_test.go b/handler/testhelper_test.go new file mode 100644 index 00000000..0722e615 --- /dev/null +++ b/handler/testhelper_test.go @@ -0,0 +1,28 @@ +// Copyright (c) 2026 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 handler + +import "github.com/uber/tango/config" + +// allowAnyRepositoryConfigProvider resolves every remote to a fixed +// repository, mirroring the stub of the same name in controller's tests. +type allowAnyRepositoryConfigProvider struct{} + +func (allowAnyRepositoryConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{ + Remote: remote, + RepositoryID: "test-repository", + }, true +} diff --git a/controller/wireerror_test.go b/handler/wireerror_test.go similarity index 92% rename from controller/wireerror_test.go rename to handler/wireerror_test.go index ae42f33c..7569df55 100644 --- a/controller/wireerror_test.go +++ b/handler/wireerror_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package controller +package handler import ( "context" @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/controller" tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" @@ -88,10 +89,10 @@ func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) stream.EXPECT().Context().Return(context.Background()) - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), - }) + })}) // Missing BaseSha triggers a validation error classified as ERROR_USER. err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ @@ -117,10 +118,10 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Context().Return(context.Background()).AnyTimes() - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), - }) + })}) // Missing first revision triggers validation error classified as ERROR_USER. err := c.GetChangedTargets(&pb.GetChangedTargetsRequest{ @@ -146,11 +147,11 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, - }) + })}) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ diff --git a/integration/integration_test.go b/integration/integration_test.go index d8daf620..05afcf99 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -156,7 +156,7 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { GraphConfig: cfg, }) - h := handler.New(handler.Params{Controller: ctrl}) + h := handler.New(handler.Params{Logger: zl, Controller: ctrl}) grpcTransport := yarpcgrpc.NewTransport() listener, err := net.Listen("tcp", "127.0.0.1:0")