From d50f9fe3e272fd772763a7c2f7fb2a9b85174800 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 15 Mar 2019 14:54:53 -0700 Subject: [PATCH 1/4] Add test case for skaffold init walk In this change: 1. Refactor walk into its own function 2. Add sample directory structure and tests. --- pkg/skaffold/initializer/init.go | 64 ++++++----- pkg/skaffold/initializer/init_test.go | 147 ++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 29 deletions(-) diff --git a/pkg/skaffold/initializer/init.go b/pkg/skaffold/initializer/init.go index 4f2701096e7..672812303d1 100644 --- a/pkg/skaffold/initializer/init.go +++ b/pkg/skaffold/initializer/init.go @@ -77,35 +77,7 @@ func DoInit(out io.Writer, c Config) error { } } - var potentialConfigs, dockerfiles []string - - err := filepath.Walk(rootDir, 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 - } - if IsSkaffoldConfig(path) { - if !c.Force { - return fmt.Errorf("pre-existing %s found", path) - } - 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 dockerfile - if docker.ValidateDockerfile(path) { - logrus.Infof("existing dockerfile found: %s", path) - dockerfiles = append(dockerfiles, path) - } - return nil - }) - + potentialConfigs, dockerfiles, err := walk(rootDir, c.Force, docker.ValidateDockerfile) if err != nil { return err } @@ -312,3 +284,37 @@ type dockerfilePair struct { Dockerfile string ImageName string } + +func walk(dir string, force bool, validateDockerfile func(string) bool) ([]string, []string, error) { + var dockerfiles, potentialConfigs []string + 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 + } + if IsSkaffoldConfig(path) { + if !force { + return fmt.Errorf("pre-existing %s found", path) + } + 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 dockerfile + if validateDockerfile(path) { + logrus.Infof("existing dockerfile found: %s", path) + dockerfiles = append(dockerfiles, path) + } + return nil + }) + if err != nil { + return nil, nil, err + } + return potentialConfigs, dockerfiles, nil +} diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go index 12c12ed2a2d..7b354ec4cd8 100644 --- a/pkg/skaffold/initializer/init_test.go +++ b/pkg/skaffold/initializer/init_test.go @@ -18,6 +18,10 @@ package initializer import ( "bytes" + "io/ioutil" + "os" + "path/filepath" + "strings" "testing" "github.com/GoogleContainerTools/skaffold/testutil" @@ -61,3 +65,146 @@ func TestPrintAnalyzeJSON(t *testing.T) { }) } } + +func TestWalk(t *testing.T) { + emptyFile := []byte("") + tests := []struct { + name string + filesWithContents map[string][]byte + expectedConfigs []string + expectedDockerfiles []string + force bool + err bool + }{ + { + name: "shd return correct k8 configs and dockerfiles", + filesWithContents: map[string][]byte{ + "config/test.yaml": emptyFile, + "k8pod.yml": emptyFile, + "README": emptyFile, + "deploy/Dockerfile": emptyFile, + "Dockerfile": emptyFile, + }, + force: false, + expectedConfigs: []string{ + "config/test.yaml", + "k8pod.yml", + }, + expectedDockerfiles: []string{ + "Dockerfile", + "deploy/Dockerfile", + }, + err: false, + }, + { + name: "shd skip hidden dir", + filesWithContents: map[string][]byte{ + ".hidden/test.yaml": emptyFile, + "k8pod.yml": emptyFile, + "README": emptyFile, + ".hidden/Dockerfile": emptyFile, + "Dockerfile": emptyFile, + }, + force: false, + expectedConfigs: []string{ + "k8pod.yml", + }, + expectedDockerfiles: []string{ + "Dockerfile", + }, + err: false, + }, + { + name: "shd not error when skaffold.config present and force = true", + filesWithContents: map[string][]byte{ + "skaffold.yaml": []byte(`apiVersion: skaffold/v1beta6 +kind: Config +deploy: + kustomize: {}`), + "config/test.yaml": emptyFile, + "k8pod.yml": emptyFile, + "README": emptyFile, + "deploy/Dockerfile": emptyFile, + "Dockerfile": emptyFile, + }, + force: true, + expectedConfigs: []string{ + "config/test.yaml", + "k8pod.yml", + }, + expectedDockerfiles: []string{ + "Dockerfile", + "deploy/Dockerfile", + }, + err: false, + }, + { + name: "shd error when skaffold.config present and force = false", + filesWithContents: map[string][]byte{ + "config/test.yaml": emptyFile, + "k8pod.yml": emptyFile, + "README": emptyFile, + "deploy/Dockerfile": emptyFile, + "Dockerfile": emptyFile, + "skaffold.yaml": []byte(`apiVersion: skaffold/v1beta6 +kind: Config +deploy: + kustomize: {}`), + }, + force: false, + expectedConfigs: nil, + expectedDockerfiles: nil, + err: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + rootDir, err := ioutil.TempDir("", "test") + if err != nil { + t.Fatal(err) + } + createDirStructure(t, rootDir, test.filesWithContents) + potentialConfigs, dockerfiles, err := walk(rootDir, test.force, testValidDocker) + testutil.CheckErrorAndDeepEqual(t, test.err, err, + convertToAbsPath(rootDir, test.expectedConfigs), potentialConfigs) + testutil.CheckErrorAndDeepEqual(t, test.err, err, + convertToAbsPath(rootDir, test.expectedDockerfiles), dockerfiles) + os.Remove(rootDir) + }) + } +} + +func testValidDocker(path string) bool { + return strings.HasSuffix(path, "Dockerfile") +} + +func createDirStructure(t *testing.T, dir string, filesWithContents map[string][]byte) { + t.Helper() + for file, content := range filesWithContents { + // Create Directory path if it does not exist. + absPath := filepath.Join(dir, filepath.Dir(file)) + if _, err := os.Stat(absPath); err != nil { + if err := os.MkdirAll(absPath, os.ModePerm); err != nil { + t.Fatal(err) + } + } + // Create filepath with contents + f, err := os.Create(filepath.Join(dir, file)) + if err != nil { + t.Fatal(err) + } + f.Write(content) + f.Close() + } +} + +func convertToAbsPath(dir string, files []string) []string { + if files == nil { + return files + } + absPaths := make([]string, len(files)) + for i, file := range files { + absPaths[i] = filepath.Join(dir, file) + } + return absPaths +} From 457c48cee7178cd97c12b76d3e17b31ceeb7703a Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Tue, 19 Mar 2019 10:47:58 -0700 Subject: [PATCH 2/4] address code review comments --- pkg/skaffold/initializer/init_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go index 7b354ec4cd8..c4fac8df265 100644 --- a/pkg/skaffold/initializer/init_test.go +++ b/pkg/skaffold/initializer/init_test.go @@ -77,7 +77,7 @@ func TestWalk(t *testing.T) { err bool }{ { - name: "shd return correct k8 configs and dockerfiles", + name: "should return correct k8 configs and dockerfiles", filesWithContents: map[string][]byte{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, @@ -97,7 +97,7 @@ func TestWalk(t *testing.T) { err: false, }, { - name: "shd skip hidden dir", + name: "should skip hidden dir", filesWithContents: map[string][]byte{ ".hidden/test.yaml": emptyFile, "k8pod.yml": emptyFile, @@ -115,7 +115,7 @@ func TestWalk(t *testing.T) { err: false, }, { - name: "shd not error when skaffold.config present and force = true", + name: "should not error when skaffold.config present and force = true", filesWithContents: map[string][]byte{ "skaffold.yaml": []byte(`apiVersion: skaffold/v1beta6 kind: Config @@ -139,7 +139,7 @@ deploy: err: false, }, { - name: "shd error when skaffold.config present and force = false", + name: "should error when skaffold.config present and force = false", filesWithContents: map[string][]byte{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, From e9aa6e81526f4e5f59f6fc6f94419dfca40f6225 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 20 Mar 2019 11:24:38 -0700 Subject: [PATCH 3/4] move all the temp dir functionality to testutil/tmp --- pkg/skaffold/initializer/init_test.go | 80 +++++++++------------------ testutil/tmp.go | 9 +++ 2 files changed, 35 insertions(+), 54 deletions(-) diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go index c4fac8df265..3620233edb7 100644 --- a/pkg/skaffold/initializer/init_test.go +++ b/pkg/skaffold/initializer/init_test.go @@ -18,9 +18,7 @@ package initializer import ( "bytes" - "io/ioutil" "os" - "path/filepath" "strings" "testing" @@ -67,18 +65,18 @@ func TestPrintAnalyzeJSON(t *testing.T) { } func TestWalk(t *testing.T) { - emptyFile := []byte("") + emptyFile := "" tests := []struct { name string - filesWithContents map[string][]byte + filesWithContents map[string]string expectedConfigs []string expectedDockerfiles []string force bool - err bool + shouldErr bool }{ { name: "should return correct k8 configs and dockerfiles", - filesWithContents: map[string][]byte{ + filesWithContents: map[string]string{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, "README": emptyFile, @@ -94,11 +92,11 @@ func TestWalk(t *testing.T) { "Dockerfile", "deploy/Dockerfile", }, - err: false, + shouldErr: false, }, { name: "should skip hidden dir", - filesWithContents: map[string][]byte{ + filesWithContents: map[string]string{ ".hidden/test.yaml": emptyFile, "k8pod.yml": emptyFile, "README": emptyFile, @@ -112,15 +110,15 @@ func TestWalk(t *testing.T) { expectedDockerfiles: []string{ "Dockerfile", }, - err: false, + shouldErr: false, }, { name: "should not error when skaffold.config present and force = true", - filesWithContents: map[string][]byte{ - "skaffold.yaml": []byte(`apiVersion: skaffold/v1beta6 + filesWithContents: map[string]string{ + "skaffold.yaml": `apiVersion: skaffold/v1beta6 kind: Config deploy: - kustomize: {}`), + kustomize: {}`, "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, "README": emptyFile, @@ -136,39 +134,38 @@ deploy: "Dockerfile", "deploy/Dockerfile", }, - err: false, + shouldErr: false, }, { name: "should error when skaffold.config present and force = false", - filesWithContents: map[string][]byte{ + filesWithContents: map[string]string{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, "README": emptyFile, "deploy/Dockerfile": emptyFile, "Dockerfile": emptyFile, - "skaffold.yaml": []byte(`apiVersion: skaffold/v1beta6 + "skaffold.yaml": `apiVersion: skaffold/v1beta6 kind: Config deploy: - kustomize: {}`), + kustomize: {}`, }, force: false, expectedConfigs: nil, expectedDockerfiles: nil, - err: true, + shouldErr: true, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - rootDir, err := ioutil.TempDir("", "test") - if err != nil { - t.Fatal(err) - } - createDirStructure(t, rootDir, test.filesWithContents) + testDir, deleteFunc := testutil.NewTempDir(t) + rootDir := testDir.Root() + deleteFunc() + writeAllFiles(testDir, test.filesWithContents) potentialConfigs, dockerfiles, err := walk(rootDir, test.force, testValidDocker) - testutil.CheckErrorAndDeepEqual(t, test.err, err, - convertToAbsPath(rootDir, test.expectedConfigs), potentialConfigs) - testutil.CheckErrorAndDeepEqual(t, test.err, err, - convertToAbsPath(rootDir, test.expectedDockerfiles), dockerfiles) + testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, + testDir.Paths(test.expectedConfigs), potentialConfigs) + testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, + testDir.Paths(test.expectedDockerfiles), dockerfiles) os.Remove(rootDir) }) } @@ -178,33 +175,8 @@ func testValidDocker(path string) bool { return strings.HasSuffix(path, "Dockerfile") } -func createDirStructure(t *testing.T, dir string, filesWithContents map[string][]byte) { - t.Helper() - for file, content := range filesWithContents { - // Create Directory path if it does not exist. - absPath := filepath.Join(dir, filepath.Dir(file)) - if _, err := os.Stat(absPath); err != nil { - if err := os.MkdirAll(absPath, os.ModePerm); err != nil { - t.Fatal(err) - } - } - // Create filepath with contents - f, err := os.Create(filepath.Join(dir, file)) - if err != nil { - t.Fatal(err) - } - f.Write(content) - f.Close() +func writeAllFiles(tmpDir *testutil.TempDir, filesWithContents map[string]string) { + for file, contents := range filesWithContents { + tmpDir.Write(file, contents) } } - -func convertToAbsPath(dir string, files []string) []string { - if files == nil { - return files - } - absPaths := make([]string, len(files)) - for i, file := range files { - absPaths[i] = filepath.Join(dir, file) - } - return absPaths -} diff --git a/testutil/tmp.go b/testutil/tmp.go index d8cc8442323..7984c74e761 100644 --- a/testutil/tmp.go +++ b/testutil/tmp.go @@ -128,3 +128,12 @@ func (h *TempDir) failIfErr(err error) *TempDir { } return h } + +// Paths returns the paths to a list of files in the temp directory. +func (h *TempDir) Paths(files []string) []string { + var paths []string + for _, file := range files { + paths = append(paths, h.Path(file)) + } + return paths +} From 860bcebafaebc4bdc6651843efc3a17e4e9cf17d Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 21 Mar 2019 09:59:11 -0700 Subject: [PATCH 4/4] fix style --- pkg/skaffold/initializer/init_test.go | 42 +++++++++++++-------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go index 3620233edb7..dd387afd294 100644 --- a/pkg/skaffold/initializer/init_test.go +++ b/pkg/skaffold/initializer/init_test.go @@ -18,7 +18,6 @@ package initializer import ( "bytes" - "os" "strings" "testing" @@ -27,7 +26,7 @@ import ( func TestPrintAnalyzeJSON(t *testing.T) { tests := []struct { - name string + description string dockerfiles []string images []string skipBuild bool @@ -35,28 +34,28 @@ func TestPrintAnalyzeJSON(t *testing.T) { expected string }{ { - name: "dockerfile and image", + description: "dockerfile and image", dockerfiles: []string{"Dockerfile", "Dockerfile_2"}, images: []string{"image1", "image2"}, expected: "{\"dockerfiles\":[\"Dockerfile\",\"Dockerfile_2\"],\"images\":[\"image1\",\"image2\"]}", }, { - name: "no dockerfile, skip build", - images: []string{"image1", "image2"}, - skipBuild: true, - expected: "{\"images\":[\"image1\",\"image2\"]}"}, + description: "no dockerfile, skip build", + images: []string{"image1", "image2"}, + skipBuild: true, + expected: "{\"images\":[\"image1\",\"image2\"]}"}, { - name: "no dockerfile", - images: []string{"image1", "image2"}, - shouldErr: true, + description: "no dockerfile", + images: []string{"image1", "image2"}, + shouldErr: true, }, { - name: "no dockerfiles or images", - shouldErr: true, + description: "no dockerfiles or images", + shouldErr: true, }, } for _, test := range tests { - t.Run(test.name, func(t *testing.T) { + t.Run(test.description, func(t *testing.T) { out := bytes.NewBuffer([]byte{}) err := printAnalyzeJSON(out, test.skipBuild, test.dockerfiles, test.images) testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, out.String()) @@ -67,7 +66,7 @@ func TestPrintAnalyzeJSON(t *testing.T) { func TestWalk(t *testing.T) { emptyFile := "" tests := []struct { - name string + description string filesWithContents map[string]string expectedConfigs []string expectedDockerfiles []string @@ -75,7 +74,7 @@ func TestWalk(t *testing.T) { shouldErr bool }{ { - name: "should return correct k8 configs and dockerfiles", + description: "should return correct k8 configs and dockerfiles", filesWithContents: map[string]string{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, @@ -95,7 +94,7 @@ func TestWalk(t *testing.T) { shouldErr: false, }, { - name: "should skip hidden dir", + description: "should skip hidden dir", filesWithContents: map[string]string{ ".hidden/test.yaml": emptyFile, "k8pod.yml": emptyFile, @@ -113,7 +112,7 @@ func TestWalk(t *testing.T) { shouldErr: false, }, { - name: "should not error when skaffold.config present and force = true", + description: "should not error when skaffold.config present and force = true", filesWithContents: map[string]string{ "skaffold.yaml": `apiVersion: skaffold/v1beta6 kind: Config @@ -137,7 +136,7 @@ deploy: shouldErr: false, }, { - name: "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, @@ -156,17 +155,16 @@ deploy: }, } for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - testDir, deleteFunc := testutil.NewTempDir(t) + t.Run(test.description, func(t *testing.T) { + testDir, cleanUp := testutil.NewTempDir(t) + defer cleanUp() rootDir := testDir.Root() - deleteFunc() writeAllFiles(testDir, test.filesWithContents) potentialConfigs, dockerfiles, err := walk(rootDir, test.force, testValidDocker) testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, testDir.Paths(test.expectedConfigs), potentialConfigs) testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, testDir.Paths(test.expectedDockerfiles), dockerfiles) - os.Remove(rootDir) }) } }