Skip to content

refactor(controller): remove the repository resolver #336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justinwon777 merged 1 commit into from
Oct 15, 2026
Merged

refactor(controller): remove the repository resolver #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 2 additions & 31 deletions controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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,
}
Expand Down
27 changes: 0 additions & 27 deletions controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
Expand Down
6 changes: 0 additions & 6 deletions controller/getchangedtargets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
5 changes: 0 additions & 5 deletions controller/getchangedtargets_tgb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
48 changes: 18 additions & 30 deletions controller/gettargetgraph_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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,
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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"},
Expand All @@ -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,
Expand All @@ -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"},
Expand All @@ -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,
Expand Down
10 changes: 0 additions & 10 deletions controller/testhelper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand All @@ -44,7 +35,6 @@ func newTestController(logger *zap.Logger) *controller {
logger: logger,
emitter: metrics.Nop(),
maxMessageBytes: config.DefaultMaxMessageBytes,
repoConfig: allowAnyRepositoryConfigProvider{},
appCtx: context.Background(),
}
}
Expand Down
1 change: 0 additions & 1 deletion example/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ func run() error {
Storage: store,
Orchestrator: orch,
MaxMessageBytes: cfg.Service.MaxMessageBytes,
RepoConfig: cfg,
GraphConfig: cfg,
})

Expand Down
5 changes: 0 additions & 5 deletions handler/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading