From c2b4fd08842dd3e7c9e2ae2fb209d0fc5f750b75 Mon Sep 17 00:00:00 2001 From: i4k Date: Fri, 4 Oct 2024 12:51:12 +0100 Subject: [PATCH 1/4] chore: optimize check for untracked files. Signed-off-by: i4k --- git/git.go | 29 +++++++++++++++-------------- git/git_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/git/git.go b/git/git.go index 7cabe7f98..0dd6dbba7 100644 --- a/git/git.go +++ b/git/git.go @@ -585,27 +585,28 @@ func (git *Git) Pull(remote, branch string) error { } // ListUntracked lists untracked files in the directories provided in dirs. -func (git *Git) ListUntracked(dirs ...string) ([]string, error) { - args := []string{ - "--others", "--exclude-standard", - } - - if len(dirs) > 0 { - args = append(args, "--") - args = append(args, dirs...) - } - +func (git *Git) ListUntracked() ([]string, error) { log.Debug(). Str("action", "ListUntracked()"). Str("workingDir", git.cfg().WorkingDir). Msg("List untracked files.") - out, err := git.exec("ls-files", args...) + + out, err := git.exec("status", "--porcelain") if err != nil { - return nil, fmt.Errorf("ls-files: %w", err) + return nil, fmt.Errorf("git status --porcelain: %w", err) } - return removeEmptyLines(strings.Split(out, "\n")), nil - + var files []string + for _, line := range strings.Split(out, "\n") { + if strings.HasPrefix(line, "?? ") { + file := strings.TrimSpace(strings.TrimPrefix(line, "?? ")) + if len(file) > 1 && file[len(file)-1] == os.PathSeparator { + file = file[:len(file)-1] + } + files = append(files, file) + } + } + return files, nil } // ListUncommitted lists uncommitted files in the directories provided in dirs. diff --git a/git/git_test.go b/git/git_test.go index b80226e1a..181ad5560 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -462,6 +462,38 @@ func TestGetConfigValue(t *testing.T) { assert.Error(t, err, "git config: non-existing key") } +func TestListUntracked(t *testing.T) { + t.Parallel() + const ( + remote = "origin" + ) + + repodir := mkOneCommitRepo(t) + g := test.NewGitWrapper(t, repodir, []string{}) + + remoteDir := test.EmptyRepo(t, true) + + assert.NoError(t, g.RemoteAdd(remote, remoteDir)) + assert.NoError(t, g.Push(remote, defaultBranch)) + + files, err := g.ListUntracked() + assert.NoError(t, err) + assert.EqualInts(t, 0, len(files)) + + test.WriteFile(t, repodir, "test.txt", "some content") + files, err = g.ListUntracked() + assert.NoError(t, err) + assert.EqualInts(t, 1, len(files)) + assert.EqualStrings(t, "test.txt", files[0]) + + test.WriteFile(t, filepath.Join(repodir, "deep/nested/path"), "test.txt", "some content") + files, err = g.ListUntracked() + assert.NoError(t, err) + assert.EqualInts(t, 2, len(files)) + assert.EqualStrings(t, "deep", files[0]) + assert.EqualStrings(t, "test.txt", files[1]) +} + const defaultBranch = "main" func mkOneCommitRepo(t *testing.T) string { From 25d061f77128edadb31f8a633494346cf1e3f1d2 Mon Sep 17 00:00:00 2001 From: i4k Date: Fri, 4 Oct 2024 14:04:15 +0100 Subject: [PATCH 2/4] fix: fallback to file listing if change detection inside a skipped directory.. Signed-off-by: i4k --- config/config.go | 30 ++++++++++++++++++++++++------ stack/manager.go | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/config/config.go b/config/config.go index 24bdf41b7..6395c1a3f 100644 --- a/config/config.go +++ b/config/config.go @@ -62,6 +62,8 @@ type Tree struct { // Node is the configuration of this tree node. Node hcl.Config + Skipped bool // tells if this node subdirs were skipped + TerramateFiles []string OtherFiles []string @@ -154,7 +156,14 @@ func (root *Root) Tree() *Tree { return &root.tree } func (root *Root) HostDir() string { return root.tree.RootDir() } // Lookup a node from the root using a filesystem query path. -func (root *Root) Lookup(path project.Path) (*Tree, bool) { +func (root *Root) Lookup(path project.Path) (node *Tree, found bool) { + node, _, found = root.tree.lookup(path) + return node, found +} + +// Lookup2 is like Lookup but returns skipped as true if the path is not found because +// a parent directory was skipped. +func (root *Root) Lookup2(path project.Path) (node *Tree, skipped bool, found bool) { return root.tree.lookup(path) } @@ -387,10 +396,13 @@ func (tree *Tree) stacks(cond func(*Tree) bool) List[*Tree] { // Lookup a node from the tree using a filesystem query path. // The abspath is relative to the current tree node. -func (tree *Tree) lookup(abspath project.Path) (*Tree, bool) { +func (tree *Tree) lookup(abspath project.Path) (node *Tree, skipped bool, found bool) { + if tree.Skipped { + return nil, true, false + } pathstr := abspath.String() if len(pathstr) == 0 || pathstr[0] != '/' { - return nil, false + return nil, false, false } parts := strings.Split(pathstr, "/") @@ -402,11 +414,11 @@ func (tree *Tree) lookup(abspath project.Path) (*Tree, bool) { } node, found := cfg.Children[parts[i]] if !found { - return nil, false + return nil, cfg.Skipped, false } cfg = node } - return cfg, true + return cfg, false, true } // AsList returns a list with this node and all its children. @@ -439,7 +451,7 @@ func loadTree(parentTree *Tree, cfgdir string, rootcfg *hcl.Config) (_ *Tree, er for _, fname := range otherFiles { if fname == SkipFilename { logger.Debug().Msg("skip file found: skipping whole subtree") - return NewTree(cfgdir), nil + return newSkippedTree(cfgdir), nil } } @@ -618,6 +630,12 @@ func NewTree(cfgdir string) *Tree { } } +func newSkippedTree(cfgdir string) *Tree { + t := NewTree(cfgdir) + t.Skipped = true + return t +} + func (tree *Tree) hasExperiment(name string) bool { if tree.Parent != nil { return tree.Parent.hasExperiment(name) diff --git a/stack/manager.go b/stack/manager.go index fdf0cceef..c37b0d289 100644 --- a/stack/manager.go +++ b/stack/manager.go @@ -135,6 +135,12 @@ func (m *Manager) ListChanged(gitBaseRef string) (*Report, error) { } } + if len(m.cache.changedFiles) == 0 { + return &Report{ + Checks: checks, + }, nil + } + stackSet := map[project.Path]Entry{} ignoreSet := map[project.Path]struct{}{} @@ -466,12 +472,44 @@ func (m *Manager) AddWantedOf(scopeStacks config.List[*config.SortableStack]) (c } func (m *Manager) filesApply(dir project.Path, apply func(fname string) error) (err error) { - tree, ok := m.root.Lookup(dir) - if !ok { - return errors.E("directory not found: %s", dir) + var files []string + + tree, skipped, ok := m.root.Lookup2(dir) + if !ok && !skipped { + panic(errors.E(errors.ErrInternal, "path is not in the config tree and not .tmskip'ed: %s", dir)) + } + + if skipped { + // WHY: This can only happen if the user is adding a .tmskip in a modules or similar folder. + // Because of the historical performance issues in the "terramate generate" we adviced + // some customers to ".tmskip" directories with pure Terraform modules or imported only files. + // But change detection needs to track those directories as well so we fallback to listing the + // files in this case. + f, err := os.Open(dir.HostPath(m.root.HostDir())) + if err != nil { + return errors.E(err, "opening directory %q", dir) + } + + defer func() { + err = errors.L(err, f.Close()).AsError() + }() + + entries, err := f.ReadDir(-1) + if err != nil { + return errors.E(err, "listing files of directory %q", dir) + } + + for _, file := range entries { + if file.IsDir() { + continue + } + files = append(files, file.Name()) + } + } else { + files = tree.OtherFiles } - for _, fname := range tree.OtherFiles { + for _, fname := range files { err := apply(fname) if err != nil { return errors.E(err, "applying operation to file %q", fname) From ed337af94a520d831abb03e486cd4c6d99f55148 Mon Sep 17 00:00:00 2001 From: i4k Date: Fri, 4 Oct 2024 16:46:00 +0100 Subject: [PATCH 3/4] chore: improve wording of explanation. Signed-off-by: i4k --- stack/manager.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/stack/manager.go b/stack/manager.go index da316d4aa..acb106f7e 100644 --- a/stack/manager.go +++ b/stack/manager.go @@ -481,10 +481,10 @@ func (m *Manager) filesApply(dir project.Path, apply func(fname string) error) ( if skipped { // WHY: This can only happen if the user is adding a .tmskip in a modules or similar folder. - // Because of the historical performance issues in the "terramate generate" we adviced - // some customers to ".tmskip" directories with pure Terraform modules or imported only files. - // But change detection needs to track those directories as well so we fallback to listing the - // files in this case. + // The user must have a .tmskip in modules for several reasons but the most common + // are to speed up Terramate config loading or because they depend on Terraform + // modules (from other repos) that contain stack definitions that must not be recognized + // in this project. f, err := os.Open(dir.HostPath(m.root.HostDir())) if err != nil { return errors.E(err, "opening directory %q", dir) From 753e060d24fac89fd1a8ba779fe7594a98fd3bd7 Mon Sep 17 00:00:00 2001 From: i4k Date: Fri, 4 Oct 2024 16:56:01 +0100 Subject: [PATCH 4/4] chore: memoize changedFiles in a separate function. Signed-off-by: i4k --- stack/manager.go | 56 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/stack/manager.go b/stack/manager.go index acb106f7e..c0c1bb0bc 100644 --- a/stack/manager.go +++ b/stack/manager.go @@ -33,7 +33,7 @@ type ( cache struct { stacks []Entry - changedFiles []string + changedFiles map[string][]string // gitBaseRef -> changed files } } @@ -63,17 +63,21 @@ const errListChanged errors.Kind = "listing changed stacks error" // NewManager creates a new stack manager. func NewManager(root *config.Root) *Manager { - return &Manager{ + m := &Manager{ root: root, } + m.cache.changedFiles = make(map[string][]string) + return m } // NewGitAwareManager returns a stack manager that supports change detection. func NewGitAwareManager(root *config.Root, git *git.Git) *Manager { - return &Manager{ + m := &Manager{ root: root, git: git, } + m.cache.changedFiles = make(map[string][]string) + return m } // List walks the basedir directory looking for terraform stacks. @@ -126,16 +130,12 @@ func (m *Manager) ListChanged(gitBaseRef string) (*Report, error) { return nil, errors.E(errListChanged, err) } - if m.cache.changedFiles == nil { - var err error - - m.cache.changedFiles, err = m.listChangedFiles(m.root.HostDir(), gitBaseRef) - if err != nil { - return nil, errors.E(errListChanged, err) - } + changedFiles, err := m.changedFiles(gitBaseRef) + if err != nil { + return nil, errors.E(errListChanged, err) } - if len(m.cache.changedFiles) == 0 { + if len(changedFiles) == 0 { return &Report{ Checks: checks, }, nil @@ -144,7 +144,7 @@ func (m *Manager) ListChanged(gitBaseRef string) (*Report, error) { stackSet := map[project.Path]Entry{} ignoreSet := map[project.Path]struct{}{} - for _, path := range m.cache.changedFiles { + for _, path := range changedFiles { abspath := filepath.Join(m.root.HostDir(), path) projpath := project.PrjAbsPath(m.root.HostDir(), abspath) @@ -268,7 +268,7 @@ rangeStacks: continue } - if changed, ok := hasChangedWatchedFiles(stack, m.cache.changedFiles); ok { + if changed, ok := hasChangedWatchedFiles(stack, changedFiles); ok { logger.Debug(). Stringer("stack", stack). Stringer("watchfile", changed). @@ -546,7 +546,11 @@ func (m *Manager) tfModuleChanged( return false, "", errors.E("\"source\" path %q is not a directory", modAbsPath) } - for _, changedFile := range m.cache.changedFiles { + changedFiles, err := m.changedFiles(gitBaseRef) + if err != nil { + return false, "", err + } + for _, changedFile := range changedFiles { changedPath := filepath.Join(m.root.HostDir(), changedFile) if strings.HasPrefix(changedPath, modAbsPath) { return true, fmt.Sprintf("module %q has unmerged changes", mod.Source), nil @@ -592,6 +596,21 @@ func (m *Manager) tfModuleChanged( return changed, fmt.Sprintf("module %q changed because %s", mod.Source, why), nil } +func (m *Manager) changedFiles(gitBaseRef string) ([]string, error) { + changedFiles, ok := m.cache.changedFiles[gitBaseRef] + if !ok { + var err error + + changedFiles, err = m.listChangedFiles(m.root.HostDir(), gitBaseRef) + if err != nil { + return nil, errors.E(errListChanged, err) + } + + m.cache.changedFiles[gitBaseRef] = changedFiles + } + return changedFiles, nil +} + func (m *Manager) tgModuleChanged( stack *config.Stack, tgMod *tg.Module, gitBaseRef string, stackSet map[project.Path]Entry, tgModuleMap map[project.Path]*tg.Module, ) (changed bool, why string, err error) { @@ -606,6 +625,11 @@ func (m *Manager) tgModuleChanged( } } + changedFiles, err := m.changedFiles(gitBaseRef) + if err != nil { + return false, "", err + } + for _, dep := range tgMod.DependsOn { // if the module is a stack already detected as changed, just mark this as changed and // move on. Fast path. @@ -616,7 +640,7 @@ func (m *Manager) tgModuleChanged( } } - for _, changedFile := range m.cache.changedFiles { + for _, changedFile := range changedFiles { changedPath := project.PrjAbsPath(m.root.HostDir(), changedFile) if dep == changedPath { return true, fmt.Sprintf("module %q changed because %q changed", tgMod.Path, dep), nil @@ -637,7 +661,7 @@ func (m *Manager) tgModuleChanged( continue } - for _, file := range m.cache.changedFiles { + for _, file := range changedFiles { if strings.HasPrefix(filepath.Join(m.root.HostDir(), file), depAbsPath) { return true, fmt.Sprintf("module %q changed because %q changed", tgMod.Path, dep), nil }