Improve skaffold init file traversal

pkg/skaffold/initializer/init.go

@@ -30,6 +30,7 @@ import (
 	"sort"
 	"strings"
 
+	"github.com/karrick/godirwalk"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"gopkg.in/AlecAivazis/survey.v1"
@@ -110,7 +111,7 @@ func DoInit(ctx context.Context, out io.Writer, c Config) error {
 		}
 	}
 
-	potentialConfigs, builderConfigs, err := walk(rootDir, c.Force, c.EnableJibInit, detectBuilders)
+	potentialConfigs, builderConfigs, err := walk(rootDir, c.Force, c.EnableJibInit)
 	if err != nil {
 		return err
 	}
@@ -242,7 +243,10 @@ func autoSelectBuilders(builderConfigs []InitBuilder, images []string) ([]builde
 	return pairs, builderConfigs, unresolvedImages
 }
 
-func detectBuilders(enableJibInit bool, path string) ([]InitBuilder, error) {
+// detectBuilders checks if a path is a builder config, and if it is, returns the InitBuilders representing the
+// configs. Also returns a boolean marking search completion for subdirectories (true = subdirectories should
+// continue to be searched, false = subdirectories should not be searched for more builders)
+func detectBuilders(enableJibInit bool, path string) ([]InitBuilder, bool) {
 	// TODO: Remove backwards compatibility if statement (not entire block)
 	if enableJibInit {
 		// Check for jib
@@ -251,19 +255,19 @@ func detectBuilders(enableJibInit bool, path string) ([]InitBuilder, error) {
 			for i := range builders {
 				results[i] = builders[i]
 			}
-			return results, filepath.SkipDir
+			return results, false
 		}
 	}
 
 	// Check for Dockerfile
 	if docker.ValidateDockerfileFunc(path) {
 		results := []InitBuilder{docker.Docker{File: path}}
-		return results, nil
+		return results, true
 	}
 
 	// TODO: Check for more builders
 
-	return nil, nil
+	return nil, true
 }
 
 func processCliArtifacts(artifacts []string) ([]builderImagePair, error) {
@@ -538,40 +542,81 @@ func printAnalyzeJSON(out io.Writer, skipBuild bool, pairs []builderImagePair, u
 	return err
 }
 
-func walk(dir string, force, enableJibInit bool, validateBuildFile func(bool, string) ([]InitBuilder, error)) ([]string, []InitBuilder, error) {
+// walk recursively walks a directory and returns the k8s configs and builder configs that it finds
+func walk(dir string, force, enableJibInit bool) ([]string, []InitBuilder, error) {
 	var potentialConfigs []string
 	var foundBuilders []InitBuilder
-	err := filepath.Walk(dir, func(path string, f os.FileInfo, e error) error {
-		if f.IsDir() && util.IsHiddenDir(f.Name()) {
-			logrus.Debugf("skip walking hidden dir %s", f.Name())
-			return filepath.SkipDir
-		}
-		if f.IsDir() || util.IsHiddenFile(f.Name()) {
-			return nil
+
+	var searchConfigsAndBuilders func(path string, findBuilders bool) error
+	searchConfigsAndBuilders = func(path string, findBuilders bool) error {
+		dirents, err := godirwalk.ReadDirents(path, nil)
+		if err != nil {
+			return err
 		}
-		if IsSkaffoldConfig(path) {
-			if !force {
-				return fmt.Errorf("pre-existing %s found", path)
+
+		var subdirectories []*godirwalk.Dirent
+		searchForBuildersInSubdirectories := findBuilders
+		sort.Sort(dirents)
+
+		// Traverse files
+		for _, file := range dirents {
+			if util.IsHiddenFile(file.Name()) || util.IsHiddenDir(file.Name()) {
+				continue
+			}
+
+			// If we found a directory, keep track of it until we've gone through all the files first
+			if file.IsDir() {
+				subdirectories = append(subdirectories, file)
+				continue
+			}
+
+			// Check for skaffold.yaml/k8s manifest
+			filePath := filepath.Join(path, file.Name())
+			var foundConfig bool
+			if foundConfig, err = checkConfigFile(filePath, force, &potentialConfigs); err != nil {
+				return err
+			}
+
+			// Check for builder config
+			if !foundConfig && findBuilders {
+				builderConfigs, continueSearchingBuilders := detectBuilders(enableJibInit, filePath)
+				foundBuilders = append(foundBuilders, builderConfigs...)
+				searchForBuildersInSubdirectories = searchForBuildersInSubdirectories && continueSearchingBuilders
 			}
-			logrus.Debugf("%s is a valid skaffold configuration: continuing since --force=true", path)
-			return nil
-		}
-		if IsSupportedKubernetesFileExtension(path) {
-			potentialConfigs = append(potentialConfigs, path)
-			return nil
 		}
-		// try and parse build file
-		if builderConfigs, err := validateBuildFile(enableJibInit, path); builderConfigs != nil {
-			for _, buildConfig := range builderConfigs {
-				logrus.Infof("existing builder found: %s", buildConfig.Describe())
-				foundBuilders = append(foundBuilders, buildConfig)
+
+		// Recurse into subdirectories
+		for _, dir := range subdirectories {
+			if err = searchConfigsAndBuilders(filepath.Join(path, dir.Name()), searchForBuildersInSubdirectories); err != nil {
+				return err
 			}
-			return err
 		}
+
 		return nil
-	})
+	}
+
+	err := searchConfigsAndBuilders(dir, true)
 	if err != nil {
 		return nil, nil, err
 	}
 	return potentialConfigs, foundBuilders, nil
 }
+
+// checkConfigFile checks if filePath is a skaffold config or k8s config, or builder config. Detected k8s configs are added to potentialConfigs.
+// Returns true if filePath is a config file, and false if not.
+func checkConfigFile(filePath string, force bool, potentialConfigs *[]string) (bool, error) {
+	if IsSkaffoldConfig(filePath) {
+		if !force {
+			return true, fmt.Errorf("pre-existing %s found", filePath)
+		}
+		logrus.Debugf("%s is a valid skaffold configuration: continuing since --force=true", filePath)
+		return true, nil
+	}
+
+	if IsSupportedKubernetesFileExtension(filePath) {
+		*potentialConfigs = append(*potentialConfigs, filePath)
+		return true, nil
+	}
+
+	return false, nil
+}

pkg/skaffold/initializer/init_test.go

@@ -143,8 +143,8 @@ func TestWalk(t *testing.T) {
 			},
 			force: false,
 			expectedConfigs: []string{
-				"config/test.yaml",
 				"k8pod.yml",
+				"config/test.yaml",
 			},
 			expectedPaths: []string{
 				"Dockerfile",
@@ -166,8 +166,8 @@ func TestWalk(t *testing.T) {
 			force:         false,
 			enableJibInit: true,
 			expectedConfigs: []string{
-				"config/test.yaml",
 				"k8pod.yml",
+				"config/test.yaml",
 			},
 			expectedPaths: []string{
 				"Dockerfile",
@@ -184,21 +184,44 @@ func TestWalk(t *testing.T) {
 				"k8pod.yml":                      emptyFile,
 				"gradle/build.gradle":            emptyFile,
 				"gradle/subproject/build.gradle": emptyFile,
+				"maven/asubproject/pom.xml":      emptyFile,
 				"maven/pom.xml":                  emptyFile,
-				"maven/subproject/pom.xml":       emptyFile,
 			},
 			force:         false,
 			enableJibInit: true,
 			expectedConfigs: []string{
-				"config/test.yaml",
 				"k8pod.yml",
+				"config/test.yaml",
 			},
 			expectedPaths: []string{
 				"gradle/build.gradle",
 				"maven/pom.xml",
 			},
 			shouldErr: false,
 		},
+		{
+			description: "multiple builders in same directory",
+			filesWithContents: map[string]string{
+				"build.gradle":                 emptyFile,
+				"ignored-builder/build.gradle": emptyFile,
+				"not-ignored-config/test.yaml": emptyFile,
+				"Dockerfile":                   emptyFile,
+				"k8pod.yml":                    emptyFile,
+				"pom.xml":                      emptyFile,
+			},
+			force:         false,
+			enableJibInit: true,
+			expectedConfigs: []string{
+				"k8pod.yml",
+				"not-ignored-config/test.yaml",
+			},
+			expectedPaths: []string{
+				"Dockerfile",
+				"build.gradle",
+				"pom.xml",
+			},
+			shouldErr: false,
+		},
 		{
 			description: "should skip hidden dir",
 			filesWithContents: map[string]string{
@@ -234,8 +257,8 @@ deploy:
 			force:         true,
 			enableJibInit: true,
 			expectedConfigs: []string{
-				"config/test.yaml",
 				"k8pod.yml",
+				"config/test.yaml",
 			},
 			expectedPaths: []string{
 				"Dockerfile",
@@ -244,7 +267,7 @@ deploy:
 			shouldErr: false,
 		},
 		{
-			description: "should  error when skaffold.config present and force = false",
+			description: "should error when skaffold.config present and force = false",
 			filesWithContents: map[string]string{
 				"config/test.yaml":  emptyFile,
 				"k8pod.yml":         emptyFile,
@@ -253,6 +276,24 @@ deploy:
 				"Dockerfile":        emptyFile,
 				"skaffold.yaml": `apiVersion: skaffold/v1beta6
 kind: Config
+deploy:
+  kustomize: {}`,
+			},
+			force:           false,
+			enableJibInit:   true,
+			expectedConfigs: nil,
+			expectedPaths:   nil,
+			shouldErr:       true,
+		},
+		{
+			description: "should error when skaffold.config present with jib config",
+			filesWithContents: map[string]string{
+				"config/test.yaml": emptyFile,
+				"k8pod.yml":        emptyFile,
+				"README":           emptyFile,
+				"pom.xml":          emptyFile,
+				"skaffold.yaml": `apiVersion: skaffold/v1beta6
+kind: Config
 deploy:
   kustomize: {}`,
 			},
@@ -265,15 +306,18 @@ deploy:
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			tmpDir := t.NewTempDir().
-				WriteFiles(test.filesWithContents)
+			tmpDir := t.NewTempDir().WriteFiles(test.filesWithContents)
 
 			t.Override(&docker.ValidateDockerfileFunc, fakeValidateDockerfile)
 			t.Override(&jib.ValidateJibConfigFunc, fakeValidateJibConfig)
 
-			potentialConfigs, builders, err := walk(tmpDir.Root(), test.force, test.enableJibInit, detectBuilders)
+			potentialConfigs, builders, err := walk(tmpDir.Root(), test.force, test.enableJibInit)
 
 			t.CheckError(test.shouldErr, err)
+			if test.shouldErr {
+				return
+			}
+
 			t.CheckDeepEqual(tmpDir.Paths(test.expectedConfigs...), potentialConfigs)
 			t.CheckDeepEqual(len(test.expectedPaths), len(builders))
 			for i := range builders {