Updated unit tests after refactor

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) {

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
 		}

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) {
+func DestinationFilepath(src, dest, cwd string) (string, error) {
-	fi, err := os.Lstat(filepath.Join(buildcontext, filename))
+	if IsDestDir(dest) {
-	if err != nil {
+		destPath := filepath.Join(dest, filepath.Base(src))
-		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)
 		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 IsSrcsValid(srcsAndDest instructions.SourcesAndDest, resolvedSources []string, root string) error {
-func SourcesToFilesMap(srcs []string, root string) (map[string][]string, error) {
+	srcs := srcsAndDest[:len(srcsAndDest)-1]
-	srcMap := make(map[string][]string)
+	dest := srcsAndDest[len(srcsAndDest)-1]
-	for _, src := range srcs {
+
+	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 {

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
+	src              string
-	filename         string
 	dest             string
 	cwd              string
-	buildcontext     string
 	expectedFilepath string
 }{
 	{
-		srcName:          "context/foo",
+		src:              "context/foo",
-		filename:         "context/foo",
 		dest:             "/foo",
 		cwd:              "/",
 		expectedFilepath: "/foo",
 	},
 	{
-		srcName:          "context/foo",
+		src:              "context/foo",
-		filename:         "context/foo",
 		dest:             "/foodir/",
 		cwd:              "/",
 		expectedFilepath: "/foodir/foo",
 	},
 	{
-		srcName:          "context/foo",
+		src:              "context/foo",
-		filename:         "./context/foo",
 		cwd:              "/",
 		dest:             "foo",
 		expectedFilepath: "/foo",
 	},
 	{
-		srcName:          "context/bar/",
+		src:              "context/bar/",
-		filename:         "context/bar/bam/bat",
 		cwd:              "/",
 		dest:             "pkg/",
-		expectedFilepath: "/pkg/bam/bat",
+		expectedFilepath: "/pkg/bar",
 	},
 	{
-		srcName:          "context/bar/",
+		src:              "context/bar/",
-		filename:         "context/bar/bam/bat",
 		cwd:              "/newdir",
 		dest:             "pkg/",
-		expectedFilepath: "/newdir/pkg/bam/bat",
+		expectedFilepath: "/newdir/pkg/bar",
 	},
 	{
-		srcName:          "./context/empty",
+		src:              "./context/empty",
-		filename:         "context/empty",
 		cwd:              "/",
 		dest:             "/empty",
 		expectedFilepath: "/empty",
 	},
 	{
-		srcName:          "./context/empty",
+		src:              "./context/empty",
-		filename:         "context/empty",
 		cwd:              "/dir",
 		dest:             "/empty",
 		expectedFilepath: "/empty",
 	},
 	{
-		srcName:          "./",
+		src:              "./",
-		filename:         "./",
 		cwd:              "/",
 		dest:             "/dir",
 		expectedFilepath: "/dir",
 	},
 	{
-		srcName:          "./",
+		src:              "context/foo",
-		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",
 		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
+	srcsAndDest     []string
-	files       map[string][]string
+	resolvedSources []string
-	shouldErr   bool
+	shouldErr       bool
 }{
 	{
 		srcsAndDest: []string{
-			"src1",
+			"context/foo",
-			"src2",
+			"context/bar",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src1": {
+			"context/foo",
-				"file1",
+			"context/bar",
-			},
-			"src2:": {
-				"file2",
-			},
 		},
 		shouldErr: true,
 	},
 	{
 		srcsAndDest: []string{
-			"src1",
+			"context/foo",
-			"src2",
+			"context/bar",
 			"dest/",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src1": {
+			"context/foo",
-				"file1",
+			"context/bar",
-			},
-			"src2:": {
-				"file2",
-			},
 		},
 		shouldErr: false,
 	},
 	{
 		srcsAndDest: []string{
-			"src2/",
+			"context/bar/bam",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src1": {
+			"context/bar/bam",
-				"file1",
-			},
-			"src2:": {
-				"file2",
-			},
 		},
 		shouldErr: false,
 	},
 	{
 		srcsAndDest: []string{
-			"src2",
+			"context/foo",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src1": {
+			"context/foo",
-				"file1",
-			},
-			"src2:": {
-				"file2",
-			},
 		},
 		shouldErr: false,
 	},
 	{
 		srcsAndDest: []string{
-			"src2",
+			"context/foo",
-			"src*",
+			"context/b*",
 			"dest/",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src1": {
+			"context/foo",
-				"file1",
+			"context/bar",
-			},
-			"src2:": {
-				"file2",
-			},
 		},
 		shouldErr: false,
 	},
 	{
 		srcsAndDest: []string{
-			"src2",
+			"context/foo",
-			"src*",
+			"context/b*",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src2": {
+			"context/foo",
-				"src2/a",
+			"context/bar",
-				"src2/b",
-			},
-			"src*": {},
 		},
 		shouldErr: true,
 	},
 	{
 		srcsAndDest: []string{
-			"src2",
+			"context/foo",
-			"src*",
+			"context/doesntexist*",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src2": {
+			"context/foo",
-				"src2/a",
-			},
-			"src*": {},
 		},
 		shouldErr: false,
 	},
 	{
 		srcsAndDest: []string{
-			"src2",
+			"context/",
-			"src*",
 			"dest",
 		},
-		files: map[string][]string{
+		resolvedSources: []string{
-			"src2": {},
+			"context/",
-			"src*": {},
 		},
-		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
+	srcsAndDest  []string
-	expectedMap map[string][]string
+	expectedList []string
 }{
 	{
 		srcsAndDest: []string{
@@ -421,28 +359,18 @@ var testResolveSources = []struct {
 			testUrl,
 			"dest/",
 		},
-		expectedMap: map[string][]string{
+		expectedList: []string{
-			"context/foo": {
+			"context/foo",
-				"context/foo",
+			"context/bar",
-			},
+			testUrl,
-			"context/bar": {
-				"context/bar",
-				"context/bar/bam",
-				"context/bar/bam/bat",
-				"context/bar/bat",
-				"context/bar/baz",
-			},
-			testUrl: {
-				testUrl,
-			},
 		},
 	},
 }
 
 func Test_ResolveSources(t *testing.T) {
 	for _, test := range testResolveSources {
-		actualMap, err := ResolveSources(test.srcsAndDest, buildContextPath)
+		actualList, err := ResolveSources(test.srcsAndDest, buildContextPath)
-		testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedMap, actualMap)
+		testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedList, actualList)
 	}
 }
 

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 {