Refactoring on filepath.Walk (#3885)

* Extract code into helper package Signed-off-by: David Gageot <david@gageot.net> * wip Signed-off-by: David Gageot <david@gageot.net> * Feedback Signed-off-by: David Gageot <david@gageot.net>
GoogleContainerTools · Apr 7, 2020 · 61d1bba · 61d1bba
1 parent 89c907a
commit 61d1bba
Show file tree

Hide file tree

Showing 14 changed files with 417 additions and 254 deletions.
diff --git a/cmd/skaffold/app/cmd/find_configs.go b/cmd/skaffold/app/cmd/find_configs.go
@@ -23,13 +23,13 @@ import (
 	"io"
 	"strings"
 
-	"github.com/karrick/godirwalk"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
 var (
@@ -83,16 +83,16 @@ func doFindConfigs(_ context.Context, out io.Writer) error {
 func findConfigs(directory string) (map[string]string, error) {
 	pathToVersion := make(map[string]string)
 
-	err := godirwalk.Walk(directory, &godirwalk.Options{
-		Callback: func(path string, info *godirwalk.Dirent) error {
-			// Find files ending in ".yaml" and parseable to skaffold config in the specified root directory recursively.
-			if !info.IsDir() && (strings.HasSuffix(path, ".yaml") || strings.HasSuffix(path, ".yml")) {
-				if cfg, err := schema.ParseConfig(path, false); err == nil {
-					pathToVersion[path] = cfg.GetVersion()
-				}
-			}
-			return nil
-		},
+	// Find files ending in ".yaml" and parseable to skaffold config in the specified root directory recursively.
+	isYaml := func(path string, info walk.Dirent) (bool, error) {
+		return !info.IsDir() && (strings.HasSuffix(path, ".yaml") || strings.HasSuffix(path, ".yml")), nil
+	}
+
+	err := walk.From(directory).When(isYaml).Do(func(path string, _ walk.Dirent) error {
+		if cfg, err := schema.ParseConfig(path, false); err == nil {
+			pathToVersion[path] = cfg.GetVersion()
+		}
+		return nil
 	})
 
 	return pathToVersion, err

diff --git a/hack/versions/cmd/new/version.go b/hack/versions/cmd/new/version.go
@@ -29,6 +29,7 @@ import (
 	hackschema "github.com/GoogleContainerTools/skaffold/hack/versions/pkg/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
 // Before: prev -> current (latest)
@@ -45,12 +46,12 @@ func main() {
 	}
 
 	makeSchemaDir(current)
+
 	// Create a package for current version
-	walk(path("latest"), func(file string, info os.FileInfo) {
-		if !info.IsDir() {
-			cp(file, path(current, info.Name()))
-			sed(path(current, info.Name()), "package latest", "package "+current)
-		}
+	walk.From(path("latest")).WhenIsFile().MustDo(func(file string, info walk.Dirent) error {
+		cp(file, path(current, info.Name()))
+		sed(path(current, info.Name()), "package latest", "package "+current)
+		return nil
 	})
 
 	next := readNextVersion()
@@ -75,10 +76,9 @@ func main() {
 	hackschema.UpdateVersionComment(path("latest", "config.go"), false)
 
 	// Update skaffold.yaml in integration tests
-	walk("integration", func(path string, info os.FileInfo) {
-		if info.Name() == "skaffold.yaml" {
-			sed(path, current, next)
-		}
+	walk.From("integration").WhenHasName("skaffold.yaml").MustDo(func(path string, _ walk.Dirent) error {
+		sed(path, current, next)
+		return nil
 	})
 
 	// Add the new version to the list of versions
@@ -164,15 +164,3 @@ func lines(path string) []string {
 	}
 	return lines
 }
-
-func walk(root string, fn func(path string, info os.FileInfo)) {
-	if err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
-		if err != nil {
-			return err
-		}
-		fn(path, info)
-		return nil
-	}); err != nil {
-		panic("unable to list files")
-	}
-}
diff --git a/hack/versions/cmd/update_comments/main.go b/hack/versions/cmd/update_comments/main.go
@@ -17,26 +17,25 @@ limitations under the License.
 package main
 
 import (
-	"os"
 	"path"
-	"path/filepath"
 	"strings"
 
 	"github.com/sirupsen/logrus"
 
 	hackschema "github.com/GoogleContainerTools/skaffold/hack/versions/pkg/schema"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
 func main() {
-	schemaDir := path.Join("pkg", "skaffold", "schema")
 	_, isReleased := hackschema.GetLatestVersion()
-	if err := filepath.Walk(schemaDir, func(path string, info os.FileInfo, err error) error {
-		if info.Name() == "config.go" {
-			released := !strings.Contains(path, "latest") || isReleased
-			return hackschema.UpdateVersionComment(path, released)
-		}
-		return nil
-	}); err != nil {
+
+	updateVersionComment := func(path string, _ walk.Dirent) error {
+		released := !strings.Contains(path, "latest") || isReleased
+		return hackschema.UpdateVersionComment(path, released)
+	}
+
+	schemaDir := path.Join("pkg", "skaffold", "schema")
+	if err := walk.From(schemaDir).WhenHasName("config.go").Do(updateVersionComment); err != nil {
 		logrus.Fatalf("%s", err)
 	}
 }
diff --git a/pkg/skaffold/build/jib/jib.go b/pkg/skaffold/build/jib/jib.go
@@ -31,12 +31,12 @@ import (
 	"time"
 
 	"github.com/google/go-containerregistry/pkg/name"
-	"github.com/karrick/godirwalk"
 	"github.com/sirupsen/logrus"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
 const (
@@ -249,23 +249,27 @@ func walkFiles(workspace string, watchedFiles []string, ignoredFiles []string, c
 			continue
 		}
 
+		notIgnored := func(path string, info walk.Dirent) (bool, error) {
+			if isIgnored(path, ignoredFiles) {
+				return false, filepath.SkipDir
+			}
+
+			return true, nil
+		}
+
 		// Process directory
-		if err = godirwalk.Walk(dep, &godirwalk.Options{
-			Unsorted: true,
-			Callback: func(path string, _ *godirwalk.Dirent) error {
-				if isIgnored(path, ignoredFiles) {
-					return filepath.SkipDir
-				}
-				if info, err := os.Stat(path); err == nil && !info.IsDir() {
-					// try to relativize the path: an error indicates that the file cannot
-					// be made relative to the roots, and so we just use the full path
-					if relative, err := relativize(path, workspaceRoots...); err == nil {
-						path = relative
-					}
-					return callback(path, info)
-				}
-				return nil
-			},
+		if err = walk.From(dep).Unsorted().When(notIgnored).WhenIsFile().Do(func(path string, info walk.Dirent) error {
+			stat, err := os.Stat(path)
+			if err != nil {
+				return nil // Ignore
+			}
+
+			// try to relativize the path: an error indicates that the file cannot
+			// be made relative to the roots, and so we just use the full path
+			if relative, err := relativize(path, workspaceRoots...); err == nil {
+				path = relative
+			}
+			return callback(path, stat)
 		}); err != nil {
 			return fmt.Errorf("filepath walk: %w", err)
 		}

diff --git a/pkg/skaffold/build/list/list.go b/pkg/skaffold/build/list/list.go
@@ -21,13 +21,14 @@ import (
 	"path/filepath"
 	"sort"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 )
 
 // Files list files in a workspace, given a list of patterns and exclusions.
-// A pattern that doesn't correspond to any file is an error.
 func Files(workspace string, patterns, excludes []string) ([]string, error) {
-	var paths []string
+	notExcluded := notExcluded(workspace, excludes)
+
+	var dependencies []string
 
 	for _, pattern := range patterns {
 		expanded, err := filepath.Glob(filepath.Join(workspace, pattern))
@@ -39,26 +40,47 @@ func Files(workspace string, patterns, excludes []string) ([]string, error) {
 			return nil, fmt.Errorf("pattern %q did not match any file", pattern)
 		}
 
-		for _, e := range expanded {
-			rel, err := filepath.Rel(workspace, e)
-			if err != nil {
-				return nil, err
-			}
+		for _, absFrom := range expanded {
+			if err := walk.From(absFrom).Unsorted().When(notExcluded).WhenIsFile().Do(func(path string, info walk.Dirent) error {
+				relPath, err := filepath.Rel(workspace, path)
+				if err != nil {
+					return err
+				}
 
-			paths = append(paths, rel)
+				dependencies = append(dependencies, relPath)
+				return nil
+			}); err != nil {
+				return nil, fmt.Errorf("walking %q: %w", absFrom, err)
+			}
 		}
 	}
 
-	files, err := docker.WalkWorkspace(workspace, excludes, paths)
-	if err != nil {
-		return nil, fmt.Errorf("walking workspace %q: %w", workspace, err)
-	}
-
-	var dependencies []string
-	for file := range files {
-		dependencies = append(dependencies, file)
-	}
-
 	sort.Strings(dependencies)
 	return dependencies, nil
 }
+
+// notExcluded creates a walk.Predicate that matches file system entries
+// only if they don't match a list of exclusion patterns.
+func notExcluded(workspace string, excludes []string) walk.Predicate {
+	return func(path string, info walk.Dirent) (bool, error) {
+		relPath, err := filepath.Rel(workspace, path)
+		if err != nil {
+			return false, err
+		}
+
+		for _, exclude := range excludes {
+			matches, err := filepath.Match(exclude, relPath)
+			if err != nil {
+				return false, err
+			}
+			if matches {
+				if info.IsDir() {
+					return false, filepath.SkipDir
+				}
+				return false, nil
+			}
+		}
+
+		return true, nil
+	}
+}
diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go
@@ -45,6 +45,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/walk"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
 )
 
@@ -162,25 +163,16 @@ func (h *HelmDeployer) Dependencies() ([]string, error) {
 		}
 
 		chartDepsDir := filepath.Join(release.ChartPath, "charts")
-		err := filepath.Walk(release.ChartPath, func(path string, info os.FileInfo, err error) error {
-			if err != nil {
-				return fmt.Errorf("failure accessing path %q: %w", path, err)
-			}
-
-			if !info.IsDir() {
-				if !strings.HasPrefix(path, chartDepsDir) || r.SkipBuildDependencies {
-					// We can always add a dependency if it is not contained in our chartDepsDir.
-					// However, if the file is in  our chartDepsDir, we can only include the file
-					// if we are not running the helm dep build phase, as that modifies files inside
-					// the chartDepsDir and results in an infinite build loop.
-					deps = append(deps, path)
-				}
-			}
 
-			return nil
-		})
+		// We can always add a dependency if it is not contained in our chartDepsDir.
+		// However, if the file is in our chartDepsDir, we can only include the file
+		// if we are not running the helm dep build phase, as that modifies files inside
+		// the chartDepsDir and results in an infinite build loop.
+		isDep := func(path string, info walk.Dirent) (bool, error) {
+			return !info.IsDir() && (!strings.HasPrefix(path, chartDepsDir) || r.SkipBuildDependencies), nil
+		}
 
-		if err != nil {
+		if err := walk.From(release.ChartPath).When(isDep).AppendPaths(&deps); err != nil {
 			return deps, fmt.Errorf("issue walking releases: %w", err)
 		}
 	}