From 10e494ef0bf19061d10b00696c35742926233eaf Mon Sep 17 00:00:00 2001 From: Justin Won Date: Thu, 15 Oct 2026 02:34:24 -0700 Subject: [PATCH] refactor(controller): GetChangedTargets takes the entity request and the repository The handler already maps the GetChangedTargets 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, and the repository configuration for its seed attributes. 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 method changes, and the mock is regenerated. The controller test TestGetChangedTargets_ValidationError goes: it passed a nil proto request, so it tested the conversion that the handler owns, and the mapper tests cover that conversion. The controller keeps its repository resolver, controller.Params.RepoConfig, and their test in this commit. The next commit removes them. Test plan: - make build, make test, make lint - handler tests check the forwarded entity request, output config, stream, and repository - controller tests build the entity request and the repository at each call site and call GetChangedTargets directly Co-Authored-By: Claude Sonnet 5.5 --- controller/BUILD.bazel | 1 - controller/README.md | 7 +- controller/controller.go | 2 +- controller/controllermock/controllermock.go | 8 +- controller/getchangedtargets.go | 26 ++----- controller/getchangedtargets_test.go | 84 ++++++--------------- controller/getchangedtargets_tgb_test.go | 20 ++--- handler/getchangedtargets.go | 6 +- handler/getchangedtargets_test.go | 14 +++- handler/metrics_test.go | 2 +- 10 files changed, 65 insertions(+), 105 deletions(-) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index dfab44b5..390e1e00 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -24,7 +24,6 @@ go_library( "//internal/targetdiff", "//internal/tgb", "//internal/tgbdiff", - "//mapper/proto", "//observability/metrics", "//orchestrator", "//tangopb", diff --git a/controller/README.md b/controller/README.md index aa516ebf..13310d4c 100644 --- a/controller/README.md +++ b/controller/README.md @@ -10,10 +10,9 @@ 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.** 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. +- **Repository input.** Each call receives an entity request and the + resolved repository configuration from the handler. The controller does + not map the request or look up the repository itself. - **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 e1fb8ed0..ee7f28e2 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -86,7 +86,7 @@ type controller struct { // configured repository that the handler resolved for the request. type Controller interface { GetTargetGraph(request entity.GetTargetGraphRequest, outputConfig *pb.OutputConfig, stream pb.TangoServiceGetTargetGraphYARPCServer, repo config.RepositoryConfig) error - GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error + GetChangedTargets(request entity.GetChangedTargetsRequest, outputConfig *pb.OutputConfig, stream pb.TangoServiceGetChangedTargetsYARPCServer, repo config.RepositoryConfig) error } // NewController creates a new controller. appCtx is cancelled on process diff --git a/controller/controllermock/controllermock.go b/controller/controllermock/controllermock.go index 74e448e3..b1c1a297 100644 --- a/controller/controllermock/controllermock.go +++ b/controller/controllermock/controllermock.go @@ -43,17 +43,17 @@ func (m *MockController) EXPECT() *MockControllerMockRecorder { } // GetChangedTargets mocks base method. -func (m *MockController) GetChangedTargets(request *tangopb.GetChangedTargetsRequest, stream tangopb.TangoServiceGetChangedTargetsYARPCServer) error { +func (m *MockController) GetChangedTargets(request entity.GetChangedTargetsRequest, outputConfig *tangopb.OutputConfig, stream tangopb.TangoServiceGetChangedTargetsYARPCServer, repo config.RepositoryConfig) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetChangedTargets", request, stream) + ret := m.ctrl.Call(m, "GetChangedTargets", request, outputConfig, stream, repo) ret0, _ := ret[0].(error) return ret0 } // GetChangedTargets indicates an expected call of GetChangedTargets. -func (mr *MockControllerMockRecorder) GetChangedTargets(request, stream any) *gomock.Call { +func (mr *MockControllerMockRecorder) GetChangedTargets(request, outputConfig, stream, repo any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChangedTargets", reflect.TypeOf((*MockController)(nil).GetChangedTargets), request, stream) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetChangedTargets", reflect.TypeOf((*MockController)(nil).GetChangedTargets), request, outputConfig, stream, repo) } // GetTargetGraph mocks base method. diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index c86a1d58..a3800a4b 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -24,7 +24,6 @@ import ( "github.com/uber/tango/config" "github.com/uber/tango/core/cachekey" - tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" "github.com/uber/tango/entity" "github.com/uber/tango/internal/mapper" @@ -33,7 +32,6 @@ import ( "github.com/uber/tango/internal/targetdiff" "github.com/uber/tango/internal/tgb" "github.com/uber/tango/internal/tgbdiff" - "github.com/uber/tango/mapper/proto" "github.com/uber/tango/observability/metrics" pb "github.com/uber/tango/tangopb" "go.uber.org/zap" @@ -82,19 +80,11 @@ type job struct { // GetChangedTargets returns the changed targets between two revisions. If the // client disconnects, the stream's context is cancelled and the function // returns with context.Canceled. -func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error { - entityReq, mappingErr := proto.ProtoToGetChangedTargetsRequest(request) - if mappingErr != nil { - mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get changed targets request: %w", mappingErr)) - } - repoCfg, repo, repositoryErr := c.resolveRequestRepository(entityReq.First.Remote, mappingErr) - e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) +func (c *controller) GetChangedTargets(entityReq entity.GetChangedTargetsRequest, outputConfig *pb.OutputConfig, stream pb.TangoServiceGetChangedTargetsYARPCServer, 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), ) - if repositoryErr != nil { - return repositoryErr - } ctx, cancelLink := c.linkRequestCtx(stream.Context()) defer cancelLink() start := time.Now() @@ -105,13 +95,13 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str // entirely. When OutputConfig is supplied, take max_distance at face value — // see proto/tango.proto OutputConfig.max_distance for the wire-default caveat. maxDist := int32(-1) - if request.GetOutputConfig() != nil { - maxDist = request.GetOutputConfig().GetMaxDistance() + if outputConfig != nil { + maxDist = outputConfig.GetMaxDistance() } // Fast path: stream a previously computed result straight from cache. - if !request.GetBypassCache() { - served, err := c.serveChangedTargetsFromCache(ctx, e, logger, entityReq, request.GetOutputConfig(), stream, repoCfg.RepositoryID, maxDist, start) + if !entityReq.BypassCache { + served, err := c.serveChangedTargetsFromCache(ctx, e, logger, entityReq, outputConfig, stream, repoCfg.RepositoryID, maxDist, start) if err != nil { return fmt.Errorf("serve from cache: %w", err) } @@ -141,7 +131,7 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str c.cacheComparedTargets(logger, entityReq, repoCfg.RepositoryID, changedTargetsResponses) sendStart := time.Now() - if err := sendTrimmedChangedTargets(stream, changedTargetsResponses, maxDist, request.GetOutputConfig()); err != nil { + if err := sendTrimmedChangedTargets(stream, changedTargetsResponses, maxDist, outputConfig); err != nil { return fmt.Errorf("send response: %w", err) } sendDuration := time.Since(sendStart) diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index da6d7090..9670a108 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -242,16 +242,6 @@ func TestCompareTargetGraphs_AllTargetsFileNoTrigger(t *testing.T) { assert.Equal(t, 0, totalChanged, "no targets should be changed when AllTargetsFiles hashes match") } -func TestGetChangedTargets_ValidationError(t *testing.T) { - ctrl := gomock.NewController(t) - stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) - - c := NewController(context.Background(), Params{RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl)}) - - err := c.GetChangedTargets(nil, stream) - require.Error(t, err) -} - func TestGetChangedTargets_CacheHit(t *testing.T) { ctrl := gomock.NewController(t) stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) @@ -285,13 +275,10 @@ func TestGetChangedTargets_CacheHit(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: -1} - err := c.GetChangedTargets(request, stream) + err := c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) } @@ -317,13 +304,10 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: -1} - err := c.GetChangedTargets(request, stream) + err := c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.Error(t, err) assert.Contains(t, err.Error(), injected.Error()) } @@ -404,13 +388,10 @@ func TestGetChangedTargets_StreamSendError(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: -1} - err := c.GetChangedTargets(request, stream) + err := c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) assert.Error(t, err) select { @@ -500,13 +481,10 @@ func TestGetChangedTargets_streamChunks(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} - err := c.GetChangedTargets(request, stream) + err := c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) select { @@ -595,14 +573,13 @@ func TestGetChangedTargets_CacheWriteUsesAppCtx(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: -1} handlerDone := make(chan error, 1) - go func() { handlerDone <- c.GetChangedTargets(request, stream) }() + go func() { + handlerDone <- c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) + }() var cacheCtx context.Context select { @@ -1089,13 +1066,10 @@ func TestGetChangedTargets_CacheHitWithDistanceFilter(t *testing.T) { Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }) - request := &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: 1}, - } + request := changedTargetsRequest() + outputConfig := &pb.OutputConfig{MaxDistance: 1} - err := c.GetChangedTargets(request, stream) + err := c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")}) require.NoError(t, err) require.Len(t, sent, 2) @@ -1337,15 +1311,7 @@ func TestSendTrimmedChangedTargets_RetainsDeletedAtMaxDistanceOne(t *testing.T) assert.True(t, gotDeleted, "DELETED entry at distance 0 must survive max_distance=1") } -func changedTargetsRequest() *pb.GetChangedTargetsRequest { - return &pb.GetChangedTargetsRequest{ - FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, - SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, - OutputConfig: &pb.OutputConfig{MaxDistance: -1}, - } -} - -func changedTargetsEntityRequest() entity.GetChangedTargetsRequest { +func changedTargetsRequest() entity.GetChangedTargetsRequest { return entity.GetChangedTargetsRequest{ First: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha1"}, Second: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha2"}, @@ -1364,7 +1330,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { c.storage = st stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsEntityRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.False(t, served, "a cache miss must not be served") }) @@ -1403,7 +1369,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) // No Send expectation: a corrupt blob must not send anything to the client. - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsEntityRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.False(t, served, "a corrupt blob must trigger recompute, not a partial send") }) @@ -1437,7 +1403,7 @@ func TestServeChangedTargetsFromCache(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Send(gomock.Any()).Return(nil).Times(2) - served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsEntityRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) + served, err := c.serveChangedTargetsFromCache(t.Context(), c.emitter, c.logger, changedTargetsRequest(), &pb.OutputConfig{MaxDistance: -1}, stream, testRepositoryID("repo:go-code"), -1, time.Now()) require.NoError(t, err) assert.True(t, served, "a clean cache hit must be served") }) @@ -1447,7 +1413,7 @@ func TestFetchTargetGraphs(t *testing.T) { // BypassCache=true keeps getGraph on the orchestrator path only, so these // tests need no storage mock. bypassRequest := func() entity.GetChangedTargetsRequest { - r := changedTargetsEntityRequest() + r := changedTargetsRequest() r.BypassCache = true return r } diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index d2d1ded3..919c11b8 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -133,8 +133,8 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { }) request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} + require.NoError(t, c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")})) changed, idToName := changedTargetsSent(t, sent) require.Len(t, changed, 1, "should detect exactly the hash-flipped target") @@ -243,8 +243,8 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { }) request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} + require.NoError(t, c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")})) changed, _ := changedTargetsSent(t, sent) require.Len(t, changed, 4, "all targets from second graph should be reported as changed") @@ -288,8 +288,8 @@ func TestGetChangedTargets_TGBAllTargetsTriggerPreservesMembershipChanges(t *tes }) request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} + require.NoError(t, c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")})) changed, idToName := changedTargetsSent(t, sent) require.Len(t, changed, 4) @@ -366,8 +366,8 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { }) request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true} + require.NoError(t, c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")})) changed, _ := changedTargetsSent(t, sent) require.Len(t, changed, 1, "only the hash-flipped target should be changed") @@ -412,8 +412,8 @@ func TestGetChangedTargets_TGBMixedFormatFallsBack(t *testing.T) { }) request := changedTargetsRequest() - request.OutputConfig = &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} - require.NoError(t, c.GetChangedTargets(request, stream)) + outputConfig := &pb.OutputConfig{MaxDistance: -1, IncludeHashes: true, IncludeTags: true, IncludeAttributes: true} + require.NoError(t, c.GetChangedTargets(request, outputConfig, stream, config.RepositoryConfig{Remote: "repo:go-code", RepositoryID: testRepositoryID("repo:go-code")})) changed, idToName := changedTargetsSent(t, sent) require.Len(t, changed, 1) diff --git a/handler/getchangedtargets.go b/handler/getchangedtargets.go index dfb227fc..97809853 100644 --- a/handler/getchangedtargets.go +++ b/handler/getchangedtargets.go @@ -25,13 +25,11 @@ import ( ) func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (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 := proto.ProtoToGetChangedTargetsRequest(request) if mappingErr != nil { mappingErr = tangoerrors.NewUser(fmt.Errorf("convert get changed targets request: %w", mappingErr)) } - _, repo, repositoryErr := h.resolveRequestRepository(entityReq.First.Remote, mappingErr) + repoCfg, repo, repositoryErr := h.resolveRequestRepository(entityReq.First.Remote, mappingErr) e := h.emitter.Tagged(map[string]string{metrics.TagRepo: repo}) op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets) logger := h.logger.WithLazy(zap.String("repository", repo)) @@ -45,5 +43,5 @@ func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream if repositoryErr != nil { return repositoryErr } - return h.controller.GetChangedTargets(request, stream) + return h.controller.GetChangedTargets(entityReq, request.GetOutputConfig(), stream, repoCfg) } diff --git a/handler/getchangedtargets_test.go b/handler/getchangedtargets_test.go index 0a6bdd2b..ee109afd 100644 --- a/handler/getchangedtargets_test.go +++ b/handler/getchangedtargets_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" @@ -36,12 +38,18 @@ func TestGetChangedTargetsForwardsToController(t *testing.T) { req := &pb.GetChangedTargetsRequest{ FirstRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha1"}, SecondRevision: &pb.BuildDescription{Strategy: pb.COMPUTATION_STRATEGY_UNSET, Remote: "repo:go-code", BaseSha: "sha2"}, + OutputConfig: &pb.OutputConfig{MaxDistance: 2}, + BypassCache: true, } stream := &fakeGetChangedTargetsStream{} - ctrl.EXPECT().GetChangedTargets(gomock.Any(), gomock.Any()).DoAndReturn( - func(gotReq *pb.GetChangedTargetsRequest, gotStream pb.TangoServiceGetChangedTargetsYARPCServer) error { - assert.Same(t, req, gotReq) + ctrl.EXPECT().GetChangedTargets(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(gotReq entity.GetChangedTargetsRequest, gotOutput *pb.OutputConfig, gotStream pb.TangoServiceGetChangedTargetsYARPCServer, gotRepo config.RepositoryConfig) error { + assert.Equal(t, "sha1", gotReq.First.BaseSha) + assert.Equal(t, "sha2", gotReq.Second.BaseSha) + assert.True(t, gotReq.BypassCache) + 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 921638ff..284d4268 100644 --- a/handler/metrics_test.go +++ b/handler/metrics_test.go @@ -43,7 +43,7 @@ func TestGetTargetGraphMetrics(t *testing.T) { func TestGetChangedTargetsMetrics(t *testing.T) { ts := tally.NewTestScope("", nil) ctrl := mock_controller.NewMockController(gomock.NewController(t)) - ctrl.EXPECT().GetChangedTargets(gomock.Any(), gomock.Any()).Return(nil) + ctrl.EXPECT().GetChangedTargets(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}