From a40cf839645223a5db6617a1d805784e429b26d1 Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 9 Jul 2026 14:59:56 -0700 Subject: [PATCH 1/3] refactor(logging): replace log.Printf with injected zap logger The git package and itg/graph used the stdlib log.Printf for best-effort skip warnings instead of the zap logger threaded through the rest of the service. - core/git: inject a *zap.SugaredLogger into the git impl via New (nil-safe, defaults to a no-op logger). The two FileHashes parse-skip warnings now use logger.Warnw. The GitFactory seam is unchanged: callers capture the logger in a closure at the wiring sites. - core/itg/graph: drop the log.Printf in GetReverseDepsAsTargets. It was a caller-input condition already handled by the continue, on a test-only path, so no logger is added to the OptimizedGraph data struct. --- core/git/BUILD.bazel | 6 +++++- core/git/git.go | 16 +++++++++++----- core/git/git_test.go | 2 ++ core/itg/graph/graph.go | 2 -- core/repomanager/repo_manager.go | 2 +- example/main.go | 4 ++-- orchestrator/native_orchestrator.go | 2 +- 7 files changed, 22 insertions(+), 12 deletions(-) diff --git a/core/git/BUILD.bazel b/core/git/BUILD.bazel index f7c3f97f..107b1e05 100644 --- a/core/git/BUILD.bazel +++ b/core/git/BUILD.bazel @@ -5,7 +5,10 @@ go_library( srcs = ["git.go"], importpath = "github.com/uber/tango/core/git", visibility = ["//visibility:public"], - deps = ["//core/execcmd"], + deps = [ + "//core/execcmd", + "@org_uber_go_zap//:zap", + ], ) go_test( @@ -15,5 +18,6 @@ go_test( deps = [ "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_uber_go_zap//:zap", ], ) diff --git a/core/git/git.go b/core/git/git.go index 27203523..043f8a37 100644 --- a/core/git/git.go +++ b/core/git/git.go @@ -21,7 +21,6 @@ import ( "encoding/hex" "errors" "fmt" - "log" "os" "os/exec" "strconv" @@ -29,6 +28,7 @@ import ( "time" "github.com/uber/tango/core/execcmd" + "go.uber.org/zap" ) const ( @@ -63,13 +63,19 @@ type Interface interface { type impl struct { directory string runner commandRunner + logger *zap.SugaredLogger } -// New creates new Git interface implementation -func New(directory string) Interface { +// New creates new Git interface implementation. A nil logger is tolerated and +// discards log output. +func New(directory string, logger *zap.SugaredLogger) Interface { + if logger == nil { + logger = zap.NewNop().Sugar() + } return &impl{ directory: directory, runner: &osExecRunner{}, + logger: logger, } } @@ -205,12 +211,12 @@ func (c *impl) FileHashes(ctx context.Context, ref string) (map[string][]byte, e x := strings.Split(line, "\t") parts := strings.Fields(x[0]) // strings.Split is guaranteed to return an array of length 1 if len(parts) < 3 || len(x) < 2 { - log.Printf("skipping %q due to unexpected format\n", line) + c.logger.Warnw("skipping ls-tree line due to unexpected format", "line", line) continue } hash, err := hex.DecodeString(parts[2]) if err != nil { - log.Printf("skipping %q due to parsing error %v\n", line, err) + c.logger.Warnw("skipping ls-tree line due to parsing error", "line", line, zap.Error(err)) continue } fileHashes[x[1]] = hash diff --git a/core/git/git_test.go b/core/git/git_test.go index 72ebba84..5514593d 100644 --- a/core/git/git_test.go +++ b/core/git/git_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" ) type runnerCall struct { @@ -253,6 +254,7 @@ func TestDefaultGit_FileHashes(t *testing.T) { g := &impl{ directory: "/repo", runner: m, + logger: zap.NewNop().Sugar(), } m.out = tt.giveOutput m.err = tt.wantError diff --git a/core/itg/graph/graph.go b/core/itg/graph/graph.go index 924f2dfc..58ed61b8 100644 --- a/core/itg/graph/graph.go +++ b/core/itg/graph/graph.go @@ -15,7 +15,6 @@ package graph import ( - "log" "maps" "slices" @@ -245,7 +244,6 @@ func (g *OptimizedGraph) GetReverseDepsAsTargets(targetNames []string) []targeth for _, targetName := range targetNames { targetID, ok := g.TargetNameToID[targetName] if !ok { - log.Printf("starting target %s is not in the graph", targetName) continue } reverseDepIDs = g.getNewReverseDeps(targetID, reverseDepIDs, visited) diff --git a/core/repomanager/repo_manager.go b/core/repomanager/repo_manager.go index 337439b1..055dab08 100644 --- a/core/repomanager/repo_manager.go +++ b/core/repomanager/repo_manager.go @@ -157,7 +157,7 @@ func (r *repoManager) Lease(ctx context.Context, desc tangopb.BuildDescription) slot.created = true } - repoGit := git.New(slot.dir) + repoGit := git.New(slot.dir, r.logger) return workspace.NewWorkspace(workspace.WorkspaceParams{ Path: slot.dir, Git: repoGit, diff --git a/example/main.go b/example/main.go index 05a3036b..51a93a28 100644 --- a/example/main.go +++ b/example/main.go @@ -83,7 +83,7 @@ func run() error { defer os.RemoveAll(workerRootPath) rm, err := repomanager.NewRepoManager(appCtx, repomanager.Params{ - Git: git.New(repoManagerClonePath), + Git: git.New(repoManagerClonePath, logger), Logger: logger, RepoManagerClonePath: repoManagerClonePath, WorkerRootPath: workerRootPath, @@ -96,7 +96,7 @@ func run() error { Storage: store, RepoManager: rm, Logger: logger, - GitFactory: git.New, + GitFactory: func(dir string) git.Interface { return git.New(dir, logger) }, Config: cfg, }) if err != nil { diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index f46c2ab0..648e79d4 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -144,7 +144,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget requests := make([]workspace.Request, 0, len(param.Req.BuildDescription.Requests)) factory := b.gitFactory if factory == nil { - factory = git.New + factory = func(dir string) git.Interface { return git.New(dir, b.logger) } } gitModule := factory(ws.Path()) From 42864cd12da715ce61c11324121622bb13adf9ed Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 9 Jul 2026 16:15:32 -0700 Subject: [PATCH 2/3] refactor(orchestrator): rename local factory var to gitFactory Match the struct field and param names for consistency. --- orchestrator/native_orchestrator.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index 648e79d4..dda336e2 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -142,12 +142,12 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget logger.Infow("GetTargetGraph: Checked out base revision") requests := make([]workspace.Request, 0, len(param.Req.BuildDescription.Requests)) - factory := b.gitFactory - if factory == nil { - factory = func(dir string) git.Interface { return git.New(dir, b.logger) } + gitFactory := b.gitFactory + if gitFactory == nil { + gitFactory = func(dir string) git.Interface { return git.New(dir, b.logger) } } - gitModule := factory(ws.Path()) + gitModule := gitFactory(ws.Path()) for _, req := range param.Req.BuildDescription.Requests { request, err := workspace.NewRequest(req.GetUrl(), gitModule, param.Req.BuildDescription.BaseSha, req.GetCommit(), logger) if err != nil { From d655931493d9d37c6dc3b343f5c1d62d7fa92c4f Mon Sep 17 00:00:00 2001 From: Xiaoyang Tan Date: Thu, 9 Jul 2026 17:33:37 -0700 Subject: [PATCH 3/3] refactor(itg/graph): remove unused GetReverseDepsAsTargets GetReverseDepsAsTargets (and its private helper getNewReverseDeps) had no non-test callers. Remove them along with their tests. --- core/itg/graph/graph.go | 31 ------------------- core/itg/graph/graph_test.go | 60 ------------------------------------ 2 files changed, 91 deletions(-) diff --git a/core/itg/graph/graph.go b/core/itg/graph/graph.go index 58ed61b8..89b85eb1 100644 --- a/core/itg/graph/graph.go +++ b/core/itg/graph/graph.go @@ -236,24 +236,6 @@ func (g *OptimizedGraph) AddTarget(target *targethasher.Target) { } } -// GetReverseDepsAsTargets returns the reverse dependencies of the given targets. -func (g *OptimizedGraph) GetReverseDepsAsTargets(targetNames []string) []targethasher.Target { - reverseDeps := make([]targethasher.Target, 0, 2*len(targetNames)) - visited := NewIntSet() - reverseDepIDs := make([]int, 0, len(targetNames)) - for _, targetName := range targetNames { - targetID, ok := g.TargetNameToID[targetName] - if !ok { - continue - } - reverseDepIDs = g.getNewReverseDeps(targetID, reverseDepIDs, visited) - } - for _, reverseDepID := range reverseDepIDs { - reverseDeps = append(reverseDeps, g.OptimizedTargetToTarget(reverseDepID)) - } - return reverseDeps -} - // OptimizedTargetToTarget converts an OptimizedTarget back to a Target. func (g *OptimizedGraph) OptimizedTargetToTarget(targetID int) targethasher.Target { name, ok := g.TargetIDToString[targetID] @@ -298,19 +280,6 @@ func (g *OptimizedGraph) OptimizedTargetToTarget(targetID int) targethasher.Targ return target } -func (g *OptimizedGraph) getNewReverseDeps(targetID int, reverseDeps []int, visited IntSet) []int { - if visited.Contains(targetID) { - return reverseDeps - } - - visited.Insert(targetID) - reverseDeps = append(reverseDeps, targetID) - for reverseDep := range g.OptimizedTargets[targetID].ReverseDeps { - reverseDeps = g.getNewReverseDeps(reverseDep, reverseDeps, visited) - } - return reverseDeps -} - func (g *OptimizedGraph) getOrGenerateTargetID(targetName string) int { if id, ok := g.TargetNameToID[targetName]; ok { return id diff --git a/core/itg/graph/graph_test.go b/core/itg/graph/graph_test.go index 774654f8..a56c718b 100644 --- a/core/itg/graph/graph_test.go +++ b/core/itg/graph/graph_test.go @@ -247,66 +247,6 @@ func TestOptimizedGraphCopy(t *testing.T) { assert.NotEqual(t, []byte{0xFF}, g.OptimizedTargets[bID].Hash) } -// --- GetReverseDepsAsTargets --- - -func TestGetReverseDepsAsTargets(t *testing.T) { - t.Parallel() - - t.Run("starting target included in results", func(t *testing.T) { - t.Parallel() - g := OptimizeGraph(map[string]*targethasher.Target{ - "//pkg:a": {Name: "//pkg:a", RuleType: "go_library"}, - }) - result := g.GetReverseDepsAsTargets([]string{"//pkg:a"}) - require.Len(t, result, 1) - assert.Equal(t, "//pkg:a", result[0].Name) - }) - - t.Run("transitive reverse deps all returned", func(t *testing.T) { - t.Parallel() - // a ← b ← c; starting from a should yield a, b, c - targets := map[string]*targethasher.Target{ - "//pkg:a": {Name: "//pkg:a", RuleType: "go_library"}, - "//pkg:b": {Name: "//pkg:b", RuleType: "go_library", Deps: []string{"//pkg:a"}}, - "//pkg:c": {Name: "//pkg:c", RuleType: "go_library", Deps: []string{"//pkg:b"}}, - } - g := OptimizeGraph(targets) - result := g.GetReverseDepsAsTargets([]string{"//pkg:a"}) - - names := make(map[string]struct{}, len(result)) - for _, t := range result { - names[t.Name] = struct{}{} - } - assert.Contains(t, names, "//pkg:a") - assert.Contains(t, names, "//pkg:b") - assert.Contains(t, names, "//pkg:c") - }) - - t.Run("unrelated targets not returned", func(t *testing.T) { - t.Parallel() - targets := map[string]*targethasher.Target{ - "//pkg:a": {Name: "//pkg:a", RuleType: "go_library"}, - "//pkg:b": {Name: "//pkg:b", RuleType: "go_library", Deps: []string{"//pkg:a"}}, - "//pkg:unrelated": {Name: "//pkg:unrelated", RuleType: "go_library"}, - } - g := OptimizeGraph(targets) - result := g.GetReverseDepsAsTargets([]string{"//pkg:a"}) - - names := make(map[string]struct{}, len(result)) - for _, t := range result { - names[t.Name] = struct{}{} - } - assert.NotContains(t, names, "//pkg:unrelated") - }) - - t.Run("unknown target is skipped", func(t *testing.T) { - t.Parallel() - g := OptimizeGraph(nil) - result := g.GetReverseDepsAsTargets([]string{"//pkg:missing"}) - assert.Empty(t, result) - }) -} - // --- OptimizedTargetToTarget --- func TestOptimizedTargetToTarget(t *testing.T) {