Merge pull request #2494 from dgageot/simpler-faster-fidn-configs

Simpler faster find configs

cmd/skaffold/app/cmd/commands.go

@@ -31,6 +31,7 @@ type Builder interface {
 	WithExample(comment, command string) Builder
 	WithFlags(adder func(*pflag.FlagSet)) Builder
 	WithCommonFlags() Builder
+	Hidden() Builder
 	ExactArgs(argCount int, action func(io.Writer, []string) error) *cobra.Command
 	NoArgs(action func(io.Writer) error) *cobra.Command
 }
@@ -76,6 +77,11 @@ func (b *builder) WithFlags(adder func(*pflag.FlagSet)) Builder {
 	return b
 }
 
+func (b *builder) Hidden() Builder {
+	b.cmd.Hidden = true
+	return b
+}
+
 func (b *builder) ExactArgs(argCount int, action func(io.Writer, []string) error) *cobra.Command {
 	b.cmd.Args = cobra.ExactArgs(argCount)
 	b.cmd.RunE = func(_ *cobra.Command, args []string) error {

cmd/skaffold/app/cmd/commands_test.go

@@ -113,6 +113,14 @@ func TestNewCmdWithCommonFlags(t *testing.T) {
 	}
 }
 
+func TestNewCmdHidden(t *testing.T) {
+	cmd := NewCmd("").NoArgs(nil)
+	testutil.CheckDeepEqual(t, false, cmd.Hidden)
+
+	cmd = NewCmd("").Hidden().NoArgs(nil)
+	testutil.CheckDeepEqual(t, true, cmd.Hidden)
+}
+
 func listFlags(set *pflag.FlagSet) map[string]*pflag.Flag {
 	flagsByName := make(map[string]*pflag.Flag)
 

cmd/skaffold/app/cmd/find_configs.go

@@ -17,18 +17,15 @@ limitations under the License.
 package cmd
 
 import (
-	"bytes"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
-	"os"
-	"path/filepath"
 	"strings"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/karrick/godirwalk"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -40,76 +37,61 @@ var (
 
 // NewCmdFindConfigs list the skaffold config files in the specified directory.
 func NewCmdFindConfigs() *cobra.Command {
-	cmd := NewCmd("find-configs").
+	return NewCmd("find-configs").
 		WithDescription("Find in a given directory all skaffold yamls files that are parseable or upgradeable with their versions.").
 		WithFlags(func(f *pflag.FlagSet) {
 			// Default to current directory
 			f.StringVarP(&directory, "directory", "d", ".", "Root directory to lookup the config files.")
 			// Output format of this Command
 			f.StringVarP(&format, "output", "o", "table", "Result format, default to table. [(-o|--output=)json|table]")
 		}).
+		Hidden().
 		NoArgs(doFindConfigs)
-	cmd.Hidden = true
-	return cmd
 }
 
 func doFindConfigs(out io.Writer) error {
 	pathToVersion, err := findConfigs(directory)
-
 	if err != nil {
 		return err
 	}
 
 	switch format {
 	case "json":
-		jsonBytes, err := json.Marshal(pathToVersion)
-		if err != nil {
-			return err
-		}
+		encoder := json.NewEncoder(out)
+		encoder.SetIndent("", "\t")
+		return encoder.Encode(pathToVersion)
 
-		var prettyJSON bytes.Buffer
-		err = json.Indent(&prettyJSON, jsonBytes, "", "\t")
-		if err != nil {
-			return err
-		}
-		fmt.Fprintln(out, prettyJSON.String())
 	case "table":
 		pathOutLen, versionOutLen := 70, 30
 		for p, v := range pathToVersion {
-			var c color.Color
-			if v == latest.Version {
-				c = color.Default
-			} else {
+			c := color.Default
+			if v != latest.Version {
 				c = color.Green
 			}
 			c.Fprintf(out, fmt.Sprintf("%%-%ds\t%%-%ds\n", pathOutLen, versionOutLen), p, v)
 		}
+
+		return nil
+
 	default:
-		return errors.New("unsupported template")
+		return fmt.Errorf("unsupported template: %s", format)
 	}
-
-	return nil
 }
 
 func findConfigs(directory string) (map[string]string, error) {
 	pathToVersion := make(map[string]string)
 
-	err := filepath.Walk(directory,
-		func(path string, info os.FileInfo, err error) error {
-			if err != nil {
-				fmt.Println(err)
-				return err
-			}
-
+	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.Contains(path, ".yaml") || strings.Contains(path, ".yml")) {
-				cfg, err := schema.ParseConfig(path, false)
-				if err == nil {
+			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
-		})
+		},
+	})
 
 	return pathToVersion, err
 }

cmd/skaffold/app/cmd/find_configs_test.go

@@ -25,89 +25,54 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-var (
-	invalidFileName     = "invalid-skaffold.yaml"
-	validFileName       = "valid-skaffold.yaml"
-	upgradeableFileName = "upgradeable-skaffold.yaml"
-)
-
 func TestFindConfigs(t *testing.T) {
-	testutil.Run(t, "", func(tt *testutil.T) {
-		latestVersion := latest.Version
-		upgradeableVersion := v1beta7.Version
-		tmpDir1, tmpDir2 := setUpTempFiles(tt, latestVersion, upgradeableVersion)
-
-		tests := []struct {
-			flagDir                *testutil.TempDir
-			resultCounts           int
-			shouldContainsMappings map[string]string
-		}{
-			{
-				flagDir:                tmpDir1,
-				resultCounts:           2,
-				shouldContainsMappings: map[string]string{validFileName: latestVersion, upgradeableFileName: upgradeableVersion},
+	tests := []struct {
+		files    map[string]string
+		expected map[string]string
+	}{
+		{
+			files: map[string]string{
+				"valid.yml":        validYaml(latest.Version),
+				"upgradeable.yaml": validYaml(v1beta7.Version),
+				"invalid.yaml":     invalidYaml(),
 			},
-			{
-				flagDir:                tmpDir2,
-				resultCounts:           1,
-				shouldContainsMappings: map[string]string{validFileName: latestVersion},
+			expected: map[string]string{
+				"valid.yml":        latest.Version,
+				"upgradeable.yaml": v1beta7.Version,
 			},
-		}
-		for _, test := range tests {
-			pathToVersion, err := findConfigs(test.flagDir.Root())
-
-			tt.CheckErrorAndDeepEqual(false, err, len(test.shouldContainsMappings), len(pathToVersion))
-			for f, v := range test.shouldContainsMappings {
-				version, ok := pathToVersion[test.flagDir.Path(f)]
-				tt.CheckDeepEqual(true, ok)
-				tt.CheckDeepEqual(version, v)
-			}
-		}
-	})
-}
+		},
+		{
+			files: map[string]string{
+				"valid.yaml":   validYaml(latest.Version),
+				"invalid.yaml": invalidYaml(),
+			},
+			expected: map[string]string{
+				"valid.yaml": latest.Version,
+			},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, "", func(t *testutil.T) {
+			tmpDir := t.NewTempDir().WriteFiles(test.files)
 
-/*
-This helper function will generate the following file tree for testing purpose
-...
-├── tmpDir1
-│   ├── valid-skaffold.yaml
-|   ├── upgradeable-skaffold.yaml
-│   └── invalid-skaffold.yaml
-└── tmpDir2
-	├── valid-skaffold.yaml
-	└── invalid-skaffold.yaml
-*/
-func setUpTempFiles(t *testutil.T, latestVersion, upgradeableVersion string) (*testutil.TempDir, *testutil.TempDir) {
-	validYaml := fmt.Sprintf(`apiVersion: %s
-kind: Config
-build:
-  artifacts:
-  - image: docker/image
-    docker:
-      dockerfile: dockerfile.test
-`, latestVersion)
+			pathToVersion, err := findConfigs(tmpDir.Root())
 
-	upgradeableYaml := fmt.Sprintf(`apiVersion: %s
-kind: Config
-build:
-  artifacts:
-  - image: docker/image
-    docker:
-      dockerfile: dockerfile.test
-`, upgradeableVersion)
+			t.CheckNoError(err)
+			t.CheckDeepEqual(len(test.expected), len(pathToVersion))
 
-	invalidYaml := `This is invalid`
+			for f, v := range test.expected {
+				version := pathToVersion[tmpDir.Path(f)]
 
-	tmpDir1 := t.NewTempDir().WriteFiles(map[string]string{
-		invalidFileName:     invalidYaml,
-		validFileName:       validYaml,
-		upgradeableFileName: upgradeableYaml,
-	})
+				t.CheckDeepEqual(version, v)
+			}
+		})
+	}
+}
 
-	tmpDir2 := t.NewTempDir().WriteFiles(map[string]string{
-		invalidFileName: invalidYaml,
-		validFileName:   validYaml,
-	})
+func validYaml(version string) string {
+	return fmt.Sprintf("apiVersion: %s\nkind: Config", version)
+}
 
-	return tmpDir1, tmpDir2
+func invalidYaml() string {
+	return "This is invalid"
 }