From 5b3c99e4a7902b85ef1843abcc1abe7d8a9bc38a Mon Sep 17 00:00:00 2001 From: Justin Won Date: Thu, 15 Oct 2026 02:34:58 -0700 Subject: [PATCH] refactor(controller): remove the repository resolver The controller no longer uses its repository resolver, because both RPCs now take the resolved repository from the handler. This commit removes the dead code: resolveRequestRepository, unknownRepositoryMetricLabel, controller.Params.RepoConfig, and the repoConfig field of the controller. The controller test of the resolver goes with it, because the handler has its own test of the same function. The tests and the wiring in example and integration stop setting RepoConfig on controller.Params. The handler drops the comments that named its copies of the constant and the resolver as temporary. Callers that build controller.Params, such as go-code, must stop setting RepoConfig. They must set it on handler.Params instead. Test plan: - make build, make test, make lint Co-Authored-By: Claude Sonnet 5.5 --- controller/controller.go | 33 +--------------- controller/controller_test.go | 27 ------------- controller/getchangedtargets_test.go | 6 --- controller/getchangedtargets_tgb_test.go | 5 --- controller/gettargetgraph_test.go | 48 +++++++++--------------- controller/testhelper_test.go | 10 ----- example/main.go | 1 - handler/handler.go | 5 --- handler/wireerror_test.go | 11 ++---- integration/integration_test.go | 1 - 10 files changed, 24 insertions(+), 123 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index ee7f28e2..4fdd20fb 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -17,11 +17,9 @@ package controller import ( "context" "errors" - "fmt" "github.com/uber-go/tally" "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" @@ -31,49 +29,23 @@ import ( "go.uber.org/zap" ) -// Duplicated in the handler. Temporary: the controller drops this copy when it -// takes the resolved repository from the handler. -const unknownRepositoryMetricLabel = "unknown" - // Params are the parameters for the controller. type Params struct { fx.In Logger *zap.Logger Storage storage.Storage Orchestrator orchestrator.Orchestrator - Scope tally.Scope `optional:"true"` - MaxMessageBytes int `optional:"true"` - RepoConfig config.RepositoryConfigProvider + Scope tally.Scope `optional:"true"` + MaxMessageBytes int `optional:"true"` GraphConfig config.GraphConfigProvider `optional:"true"` } -// resolveRequestRepository returns the configured repository and metric label -// for a validated request. Invalid requests retain the common unknown label and -// their existing error; valid requests must exactly match the configured -// repository allowlist before controller cache I/O. -// -// Duplicated in the handler. Temporary: the controller drops this copy when it -// takes the resolved repository from the handler. -func (c *controller) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { - if requestErr != nil { - return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr - } - repo, ok := c.repoConfig.GetRepositoryConfig(remote) - if !ok { - return config.RepositoryConfig{}, unknownRepositoryMetricLabel, tangoerrors.NewUser( - fmt.Errorf("repository remote %q is not configured", remote), - ) - } - return repo, repo.RepositoryID, nil -} - type controller struct { logger *zap.Logger storage storage.Storage orchestrator orchestrator.Orchestrator emitter *metrics.Emitter maxMessageBytes int - repoConfig config.RepositoryConfigProvider graphConfig config.GraphConfigProvider // appCtx is the application lifetime; cancel it on process shutdown. @@ -103,7 +75,6 @@ func NewController(appCtx context.Context, p Params) Controller { orchestrator: p.Orchestrator, emitter: emitter, maxMessageBytes: maxMessageBytes, - repoConfig: p.RepoConfig, graphConfig: p.GraphConfig, appCtx: appCtx, } diff --git a/controller/controller_test.go b/controller/controller_test.go index 07fad1fc..2914172b 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -19,38 +19,12 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber/tango/config" tangoerrors "github.com/uber/tango/core/errors" orchestratormock "github.com/uber/tango/orchestrator/orchestratormock" "go.uber.org/mock/gomock" "go.uber.org/zap" ) -type rejectAllRepositoryConfigProvider struct{} - -func (rejectAllRepositoryConfigProvider) GetRepositoryConfig(string) (config.RepositoryConfig, bool) { - return config.RepositoryConfig{}, false -} - -func TestResolveRequestRepository(t *testing.T) { - c := &controller{repoConfig: rejectAllRepositoryConfigProvider{}} - - t.Run("request error uses unknown repository", func(t *testing.T) { - repo, label, err := c.resolveRequestRepository("ignored", assert.AnError) - assert.Empty(t, repo) - assert.Equal(t, unknownRepositoryMetricLabel, label) - assert.ErrorIs(t, err, assert.AnError) - }) - - t.Run("plain remote", func(t *testing.T) { - _, label, err := c.resolveRequestRepository("git@github.com:other/repo.git", nil) - require.Error(t, err) - assert.Equal(t, unknownRepositoryMetricLabel, label) - assert.Equal(t, tangoerrors.ErrorUser, tangoerrors.GetErrorCode(err)) - }) -} - // TestNewController_StoresAppContext verifies the caller-supplied context is // retained and is the one observed by background goroutines. func TestNewController_StoresAppContext(t *testing.T) { @@ -59,7 +33,6 @@ func TestNewController_StoresAppContext(t *testing.T) { defer cancel() c := NewController(appCtx, Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), }).(*controller) diff --git a/controller/getchangedtargets_test.go b/controller/getchangedtargets_test.go index 9670a108..fd2ce7a3 100644 --- a/controller/getchangedtargets_test.go +++ b/controller/getchangedtargets_test.go @@ -269,7 +269,6 @@ func TestGetChangedTargets_CacheHit(t *testing.T) { stream.EXPECT().Send(gomock.Any()).Return(nil).Times(2) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -298,7 +297,6 @@ func TestGetChangedTargets_TreehashReadError(t *testing.T) { Return(storage.DownloadResponse{}, injected).Times(2) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zap.NewNop(), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -382,7 +380,6 @@ func TestGetChangedTargets_StreamSendError(t *testing.T) { }) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -475,7 +472,6 @@ func TestGetChangedTargets_streamChunks(t *testing.T) { }) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -567,7 +563,6 @@ func TestGetChangedTargets_CacheWriteUsesAppCtx(t *testing.T) { appCtx, cancelApp := context.WithCancel(context.Background()) defer cancelApp() c := NewController(appCtx, Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -1060,7 +1055,6 @@ func TestGetChangedTargets_CacheHitWithDistanceFilter(t *testing.T) { }).Times(2) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: storagemock, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), diff --git a/controller/getchangedtargets_tgb_test.go b/controller/getchangedtargets_tgb_test.go index 919c11b8..8a201eba 100644 --- a/controller/getchangedtargets_tgb_test.go +++ b/controller/getchangedtargets_tgb_test.go @@ -124,7 +124,6 @@ func TestGetChangedTargets_TGBNativePath(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), // no calls expected: both graphs are cached @@ -234,7 +233,6 @@ func TestGetChangedTargets_TGBAllTargetsTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -279,7 +277,6 @@ func TestGetChangedTargets_TGBAllTargetsTriggerPreservesMembershipChanges(t *tes scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -357,7 +354,6 @@ func TestGetChangedTargets_TGBAllTargetsNoTrigger(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), @@ -403,7 +399,6 @@ func TestGetChangedTargets_TGBMixedFormatFallsBack(t *testing.T) { scope := tally.NewTestScope("", nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: st, Orchestrator: orchestratormock.NewMockOrchestrator(ctrl), diff --git a/controller/gettargetgraph_test.go b/controller/gettargetgraph_test.go index 40152a89..cd4b3e73 100644 --- a/controller/gettargetgraph_test.go +++ b/controller/gettargetgraph_test.go @@ -47,9 +47,8 @@ func TestGetTargetGraph_CacheMiss_NoSend(t *testing.T) { Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte{})}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) req := entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -71,9 +70,8 @@ func TestGetTargetGraph_StorageError_Propagates(t *testing.T) { storagemock := storagemock.NewMockStorage(ctrl) storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, expected) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -96,9 +94,8 @@ func TestGetTargetGraph_DecodeError_ReturnsError(t *testing.T) { storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser([]byte("bad-bytes"))}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -125,9 +122,8 @@ func TestGetTargetGraph_SendsWhenItemPresent(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser(buf.Bytes())}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha", @@ -153,7 +149,6 @@ func TestGetTargetGraph_TreehashNotFound_NoError(t *testing.T) { graphReader := newGraphReader(t, entity.GetTargetGraphResponse{Targets: []entity.OptimizedTarget{}}) orchestrator.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(graphReader, nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orchestrator, @@ -172,9 +167,8 @@ func TestGetTargetGraph_TreehashReadError(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: &errReadCloser{err: errors.New("readfail")}}, nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -193,9 +187,8 @@ func TestGetTargetGraph_GraphFetchError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("graph error")), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -214,9 +207,8 @@ func TestGetTargetGraph_GraphReadError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: &errReadCloser{err: errors.New("readfail")}}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -240,9 +232,8 @@ func TestGetTargetGraph_StreamSendError(t *testing.T) { storagemock.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{ReadCloser: newMockReadCloser(buf.Bytes())}, nil), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: storagemock, + Logger: zaptest.NewLogger(t), + Storage: storagemock, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -265,7 +256,6 @@ func TestGetTargetGraph_GraphNotFound_FallsThrough(t *testing.T) { graphReader := newGraphReader(t, entity.GetTargetGraphResponse{Targets: []entity.OptimizedTarget{}}) orch.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(graphReader, nil) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orch, @@ -288,9 +278,8 @@ func TestGetTargetGraph_GraphReadCancelled(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("context canceled")), ) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, }) err := c.GetTargetGraph(entity.GetTargetGraphRequest{ Build: entity.BuildDescription{Remote: "repo:go-code", BaseSha: "sha"}, @@ -309,7 +298,6 @@ func TestGetTargetGraph_OrchestratorCancelled(t *testing.T) { orch := orchestratormock.NewMockOrchestrator(ctrl) orch.EXPECT().GetTargetGraph(gomock.Any(), gomock.Any()).Return(nil, errors.New("context canceled")) c := NewController(context.Background(), Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, Orchestrator: orch, diff --git a/controller/testhelper_test.go b/controller/testhelper_test.go index 34d16b0a..384c8691 100644 --- a/controller/testhelper_test.go +++ b/controller/testhelper_test.go @@ -26,15 +26,6 @@ import ( "go.uber.org/zap" ) -type allowAnyRepositoryConfigProvider struct{} - -func (allowAnyRepositoryConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { - return config.RepositoryConfig{ - Remote: remote, - RepositoryID: "test-repository", - }, true -} - func testRepositoryID(string) string { return "test-repository" } @@ -44,7 +35,6 @@ func newTestController(logger *zap.Logger) *controller { logger: logger, emitter: metrics.Nop(), maxMessageBytes: config.DefaultMaxMessageBytes, - repoConfig: allowAnyRepositoryConfigProvider{}, appCtx: context.Background(), } } diff --git a/example/main.go b/example/main.go index e3977c0e..b57f1db8 100644 --- a/example/main.go +++ b/example/main.go @@ -105,7 +105,6 @@ func run() error { Storage: store, Orchestrator: orch, MaxMessageBytes: cfg.Service.MaxMessageBytes, - RepoConfig: cfg, GraphConfig: cfg, }) diff --git a/handler/handler.go b/handler/handler.go index a186cdc6..64ed3eea 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -27,8 +27,6 @@ import ( "go.uber.org/zap" ) -// Duplicate of the constant in the controller. Temporary: the controller -// drops its copy when it takes the resolved repository from the handler. const unknownRepositoryMetricLabel = "unknown" type Params struct { @@ -59,9 +57,6 @@ func New(p Params) pb.TangoYARPCServer { // for a validated request. Invalid requests retain the common unknown label and // their existing error; valid requests must exactly match the configured // repository allowlist before controller cache I/O. -// -// Duplicate of controller.resolveRequestRepository. Temporary: the controller -// drops its copy when it takes the resolved repository from the handler. func (h *handler) resolveRequestRepository(remote string, requestErr error) (config.RepositoryConfig, string, error) { if requestErr != nil { return config.RepositoryConfig{}, unknownRepositoryMetricLabel, requestErr diff --git a/handler/wireerror_test.go b/handler/wireerror_test.go index eaf661c5..166ea3e1 100644 --- a/handler/wireerror_test.go +++ b/handler/wireerror_test.go @@ -89,8 +89,7 @@ func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), + Logger: zaptest.NewLogger(t), })}) // Missing BaseSha triggers a validation error classified as ERROR_USER. @@ -118,8 +117,7 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream.EXPECT().Context().Return(context.Background()).AnyTimes() c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), + Logger: zaptest.NewLogger(t), })}) // Missing first revision triggers validation error classified as ERROR_USER. @@ -147,9 +145,8 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) c := New(Params{Logger: zaptest.NewLogger(t), RepoConfig: allowAnyRepositoryConfigProvider{}, Controller: controller.NewController(context.Background(), controller.Params{ - RepoConfig: allowAnyRepositoryConfigProvider{}, - Logger: zaptest.NewLogger(t), - Storage: store, + Logger: zaptest.NewLogger(t), + Storage: store, })}) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ diff --git a/integration/integration_test.go b/integration/integration_test.go index eaf805f7..2bae81a1 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -152,7 +152,6 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { Logger: zl, Storage: store, Orchestrator: orch, - RepoConfig: cfg, GraphConfig: cfg, })