From be38696c7d18b6ee5721412e0feddd5f88ac6c61 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Thu, 19 Apr 2018 17:26:32 -0700 Subject: [PATCH] Updated unit tests after refactor --- pkg/commands/add.go | 5 +- pkg/commands/copy.go | 4 +- pkg/util/command_util.go | 80 +++++--------- pkg/util/command_util_test.go | 190 +++++++++++----------------------- pkg/util/fs_util.go | 2 + 5 files changed, 93 insertions(+), 188 deletions(-) diff --git a/pkg/commands/add.go b/pkg/commands/add.go index c124661c3..2ce4a090f 100644 --- a/pkg/commands/add.go +++ b/pkg/commands/add.go @@ -53,7 +53,7 @@ func (a *AddCommand) ExecuteCommand(config *manifest.Schema2Config) error { return err } dest = resolvedEnvs[len(resolvedEnvs)-1] - // Get a map of [src]:[files rooted at src] + // Resolve wildcards and get a list of resolved sources srcs, err = util.ResolveSources(resolvedEnvs, a.buildcontext) if err != nil { return err @@ -61,10 +61,9 @@ func (a *AddCommand) ExecuteCommand(config *manifest.Schema2Config) error { var unresolvedSrcs []string // If any of the sources are local tar archives: // 1. Unpack them to the specified destination - // 2. Remove it as a source that needs to be copied over // If any of the sources is a remote file URL: // 1. Download and copy it to the specified dest - // 2. Remove it as a source that needs to be copied + // Else, add to the list of unresolved sources for _, src := range srcs { fullPath := filepath.Join(a.buildcontext, src) if util.IsSrcRemoteFileURL(src) { diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 5cdcd55ce..bded4bf99 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -45,7 +45,7 @@ func (c *CopyCommand) ExecuteCommand(config *manifest.Schema2Config) error { return err } dest = resolvedEnvs[len(resolvedEnvs)-1] - // Get a map of [src]:[files rooted at src] + // Resolve wildcards and get a list of resolved sources srcs, err = util.ResolveSources(resolvedEnvs, c.buildcontext) if err != nil { return err @@ -57,7 +57,7 @@ func (c *CopyCommand) ExecuteCommand(config *manifest.Schema2Config) error { if err != nil { return err } - destPath, err := util.DestinationFilepath(src, src, dest, config.WorkingDir, c.buildcontext) + destPath, err := util.DestinationFilepath(src, dest, config.WorkingDir) if err != nil { return err } diff --git a/pkg/util/command_util.go b/pkg/util/command_util.go index 251b96980..20efd19a3 100644 --- a/pkg/util/command_util.go +++ b/pkg/util/command_util.go @@ -83,7 +83,7 @@ func ContainsWildcards(paths []string) bool { } // ResolveSources resolves the given sources if the sources contains wildcards -// It returns a map of [src]:[files rooted at src] +// It returns a list of resolved sources func ResolveSources(srcsAndDest instructions.SourcesAndDest, root string) ([]string, error) { srcs := srcsAndDest[:len(srcsAndDest)-1] // If sources contain wildcards, we first need to resolve them to actual paths @@ -136,24 +136,9 @@ func IsDestDir(path string) bool { // If source is a dir: // Assume dest is also a dir, and copy to dest/relpath // If dest is not an absolute filepath, add /cwd to the beginning -func DestinationFilepath(filename, srcName, dest, cwd, buildcontext string) (string, error) { - fi, err := os.Lstat(filepath.Join(buildcontext, filename)) - if err != nil { - return "", err - } - src, err := os.Lstat(filepath.Join(buildcontext, srcName)) - if err != nil { - return "", err - } - if src.IsDir() || IsDestDir(dest) { - relPath, err := filepath.Rel(srcName, filename) - if err != nil { - return "", err - } - if relPath == "." && !fi.IsDir() { - relPath = filepath.Base(filename) - } - destPath := filepath.Join(dest, relPath) +func DestinationFilepath(src, dest, cwd string) (string, error) { + if IsDestDir(dest) { + destPath := filepath.Join(dest, filepath.Base(src)) if filepath.IsAbs(dest) { return destPath, nil } @@ -182,51 +167,42 @@ func URLDestinationFilepath(rawurl, dest, cwd string) string { return destPath } -// SourcesToFilesMap returns a map of [src]:[files rooted at source] -func SourcesToFilesMap(srcs []string, root string) (map[string][]string, error) { - srcMap := make(map[string][]string) - for _, src := range srcs { +func IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSources []string, root string) error { + srcs := srcsAndDest[:len(srcsAndDest)-1] + dest := srcsAndDest[len(srcsAndDest)-1] + + if !ContainsWildcards(srcs) { + if len(srcs) > 1 && !IsDestDir(dest) { + return errors.New("when specifying multiple sources in a COPY command, destination must be a directory and end in '/'") + } + } + + if len(resolvedSources) == 1 { + fi, err := os.Stat(filepath.Join(root, resolvedSources[0])) + if err != nil { + return err + } + if fi.IsDir() { + return nil + } + } + + totalFiles := 0 + for _, src := range resolvedSources { if IsSrcRemoteFileURL(src) { - srcMap[src] = []string{src} + totalFiles++ continue } src = filepath.Clean(src) files, err := RelativeFiles(src, root) if err != nil { - return nil, err + return err } - srcMap[src] = files - } - return srcMap, nil -} - -// IsSrcsValid returns an error if the sources provided are invalid, or nil otherwise -func IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSrcs []string, root string) error { - srcs := srcsAndDest[:len(srcsAndDest)-1] - dest := srcsAndDest[len(srcsAndDest)-1] - - // Now, get a map of [src]:[files rooted at src] - srcMap, err := SourcesToFilesMap(resolvedSrcs, root) - if err != nil { - return err - } - - totalFiles := 0 - for _, files := range srcMap { totalFiles += len(files) } if totalFiles == 0 { return errors.New("copy failed: no source files specified") } - - if !ContainsWildcards(srcs) { - // If multiple sources and destination isn't a directory, return an error - if len(srcs) > 1 && !IsDestDir(dest) { - return errors.New("when specifying multiple sources in a COPY command, destination must be a directory and end in '/'") - } - return nil - } - // If there are wildcards, and the destination is a file, there must be exactly one file to copy over, // Otherwise, return an error if !IsDestDir(dest) && totalFiles > 1 { diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index 242a1a305..478a4d349 100644 --- a/pkg/util/command_util_test.go +++ b/pkg/util/command_util_test.go @@ -110,93 +110,61 @@ func Test_EnvReplacement(t *testing.T) { var buildContextPath = "../../integration_tests/" var destinationFilepathTests = []struct { - srcName string - filename string + src string dest string cwd string - buildcontext string expectedFilepath string }{ { - srcName: "context/foo", - filename: "context/foo", + src: "context/foo", dest: "/foo", cwd: "/", expectedFilepath: "/foo", }, { - srcName: "context/foo", - filename: "context/foo", + src: "context/foo", dest: "/foodir/", cwd: "/", expectedFilepath: "/foodir/foo", }, { - srcName: "context/foo", - filename: "./context/foo", + src: "context/foo", cwd: "/", dest: "foo", expectedFilepath: "/foo", }, { - srcName: "context/bar/", - filename: "context/bar/bam/bat", + src: "context/bar/", cwd: "/", dest: "pkg/", - expectedFilepath: "/pkg/bam/bat", + expectedFilepath: "/pkg/bar", }, { - srcName: "context/bar/", - filename: "context/bar/bam/bat", + src: "context/bar/", cwd: "/newdir", dest: "pkg/", - expectedFilepath: "/newdir/pkg/bam/bat", + expectedFilepath: "/newdir/pkg/bar", }, { - srcName: "./context/empty", - filename: "context/empty", + src: "./context/empty", cwd: "/", dest: "/empty", expectedFilepath: "/empty", }, { - srcName: "./context/empty", - filename: "context/empty", + src: "./context/empty", cwd: "/dir", dest: "/empty", expectedFilepath: "/empty", }, { - srcName: "./", - filename: "./", + src: "./", cwd: "/", dest: "/dir", expectedFilepath: "/dir", }, { - srcName: "./", - filename: "context/foo", - cwd: "/", - dest: "/dir", - expectedFilepath: "/dir/context/foo", - }, - { - srcName: ".", - filename: "context/bar", - cwd: "/", - dest: "/dir", - expectedFilepath: "/dir/context/bar", - }, - { - srcName: ".", - filename: "context/bar", - cwd: "/", - dest: "/dir", - expectedFilepath: "/dir/context/bar", - }, - { - srcName: "context/foo", - filename: "context/foo", + src: "context/foo", cwd: "/test", dest: ".", expectedFilepath: "/test/foo", @@ -205,7 +173,7 @@ var destinationFilepathTests = []struct { func Test_DestinationFilepath(t *testing.T) { for _, test := range destinationFilepathTests { - actualFilepath, err := DestinationFilepath(test.filename, test.srcName, test.dest, test.cwd, buildContextPath) + actualFilepath, err := DestinationFilepath(test.src, test.dest, test.cwd) testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedFilepath, actualFilepath) } } @@ -278,141 +246,111 @@ func Test_MatchSources(t *testing.T) { } var isSrcValidTests = []struct { - srcsAndDest []string - files map[string][]string - shouldErr bool + srcsAndDest []string + resolvedSources []string + shouldErr bool }{ { srcsAndDest: []string{ - "src1", - "src2", + "context/foo", + "context/bar", "dest", }, - files: map[string][]string{ - "src1": { - "file1", - }, - "src2:": { - "file2", - }, + resolvedSources: []string{ + "context/foo", + "context/bar", }, shouldErr: true, }, { srcsAndDest: []string{ - "src1", - "src2", + "context/foo", + "context/bar", "dest/", }, - files: map[string][]string{ - "src1": { - "file1", - }, - "src2:": { - "file2", - }, + resolvedSources: []string{ + "context/foo", + "context/bar", }, shouldErr: false, }, { srcsAndDest: []string{ - "src2/", + "context/bar/bam", "dest", }, - files: map[string][]string{ - "src1": { - "file1", - }, - "src2:": { - "file2", - }, + resolvedSources: []string{ + "context/bar/bam", }, shouldErr: false, }, { srcsAndDest: []string{ - "src2", + "context/foo", "dest", }, - files: map[string][]string{ - "src1": { - "file1", - }, - "src2:": { - "file2", - }, + resolvedSources: []string{ + "context/foo", }, shouldErr: false, }, { srcsAndDest: []string{ - "src2", - "src*", + "context/foo", + "context/b*", "dest/", }, - files: map[string][]string{ - "src1": { - "file1", - }, - "src2:": { - "file2", - }, + resolvedSources: []string{ + "context/foo", + "context/bar", }, shouldErr: false, }, { srcsAndDest: []string{ - "src2", - "src*", + "context/foo", + "context/b*", "dest", }, - files: map[string][]string{ - "src2": { - "src2/a", - "src2/b", - }, - "src*": {}, + resolvedSources: []string{ + "context/foo", + "context/bar", }, shouldErr: true, }, { srcsAndDest: []string{ - "src2", - "src*", + "context/foo", + "context/doesntexist*", "dest", }, - files: map[string][]string{ - "src2": { - "src2/a", - }, - "src*": {}, + resolvedSources: []string{ + "context/foo", }, shouldErr: false, }, { srcsAndDest: []string{ - "src2", - "src*", + "context/", "dest", }, - files: map[string][]string{ - "src2": {}, - "src*": {}, + resolvedSources: []string{ + "context/", }, - shouldErr: true, + shouldErr: false, }, } func Test_IsSrcsValid(t *testing.T) { for _, test := range isSrcValidTests { - err := IsSrcsValid(test.srcsAndDest, test.files) + err := IsSrcsValid(test.srcsAndDest, test.resolvedSources, buildContextPath) testutil.CheckError(t, test.shouldErr, err) } } var testResolveSources = []struct { - srcsAndDest []string - expectedMap map[string][]string + srcsAndDest []string + expectedList []string }{ { srcsAndDest: []string{ @@ -421,28 +359,18 @@ var testResolveSources = []struct { testUrl, "dest/", }, - expectedMap: map[string][]string{ - "context/foo": { - "context/foo", - }, - "context/bar": { - "context/bar", - "context/bar/bam", - "context/bar/bam/bat", - "context/bar/bat", - "context/bar/baz", - }, - testUrl: { - testUrl, - }, + expectedList: []string{ + "context/foo", + "context/bar", + testUrl, }, }, } func Test_ResolveSources(t *testing.T) { for _, test := range testResolveSources { - actualMap, err := ResolveSources(test.srcsAndDest, buildContextPath) - testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedMap, actualMap) + actualList, err := ResolveSources(test.srcsAndDest, buildContextPath) + testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedList, actualList) } } diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index 60631adce..f5c96b80b 100644 --- a/pkg/util/fs_util.go +++ b/pkg/util/fs_util.go @@ -244,6 +244,7 @@ func CopyDir(src, dest string) error { return nil } +// CopySymlink copies the symlink at src to dest func CopySymlink(src, dest string) error { link, err := os.Readlink(src) if err != nil { @@ -253,6 +254,7 @@ func CopySymlink(src, dest string) error { return os.Symlink(linkDst, dest) } +// CopyFile copies the file at src to dest func CopyFile(src, dest string) error { fi, err := os.Stat(src) if err != nil {