Merge pull request #128 from priyawadhwa/refactor-copy

Refactor copy and add

pkg/commands/add.go

@@ -53,60 +53,50 @@ 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
-	srcMap, err := util.ResolveSources(resolvedEnvs, a.buildcontext)
+	srcs, err = util.ResolveSources(resolvedEnvs, a.buildcontext)
 	if err != nil {
 		return err
 	}
+	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, files := range srcMap {
+	for _, src := range srcs {
-		for _, file := range files {
+		fullPath := filepath.Join(a.buildcontext, src)
-			// If file is a local tar archive, then we unpack it to dest
+		if util.IsSrcRemoteFileURL(src) {
-			filePath := filepath.Join(a.buildcontext, file)
+			urlDest := util.URLDestinationFilepath(src, dest, config.WorkingDir)
-			isFilenameSource, err := isFilenameSource(srcMap, file)
+			logrus.Infof("Adding remote URL %s to %s", src, urlDest)
+			if err := util.DownloadFileToDest(src, urlDest); err != nil {
+				return err
+			}
+			a.snapshotFiles = append(a.snapshotFiles, urlDest)
+		} else if util.IsFileLocalTarArchive(fullPath) {
+			logrus.Infof("Unpacking local tar archive %s to %s", src, dest)
+			if err := util.UnpackLocalTarArchive(fullPath, dest); err != nil {
+				return err
+			}
+			// Add the unpacked files to the snapshotter
+			filesAdded, err := util.Files(dest)
 			if err != nil {
 				return err
 			}
-			if util.IsSrcRemoteFileURL(file) {
+			logrus.Debugf("Added %v from local tar archive %s", filesAdded, src)
-				urlDest := util.URLDestinationFilepath(file, dest, config.WorkingDir)
+			a.snapshotFiles = append(a.snapshotFiles, filesAdded...)
-				logrus.Infof("Adding remote URL %s to %s", file, urlDest)
+		} else {
-				if err := util.DownloadFileToDest(file, urlDest); err != nil {
+			unresolvedSrcs = append(unresolvedSrcs, src)
-					return err
-				}
-				a.snapshotFiles = append(a.snapshotFiles, urlDest)
-				delete(srcMap, src)
-			} else if isFilenameSource && util.IsFileLocalTarArchive(filePath) {
-				logrus.Infof("Unpacking local tar archive %s to %s", file, dest)
-				if err := util.UnpackLocalTarArchive(filePath, dest); err != nil {
-					return err
-				}
-				// Add the unpacked files to the snapshotter
-				filesAdded, err := util.Files(dest)
-				if err != nil {
-					return err
-				}
-				logrus.Debugf("Added %v from local tar archive %s", filesAdded, file)
-				a.snapshotFiles = append(a.snapshotFiles, filesAdded...)
-				delete(srcMap, src)
-			}
 		}
 	}
 	// With the remaining "normal" sources, create and execute a standard copy command
-	if len(srcMap) == 0 {
+	if len(unresolvedSrcs) == 0 {
 		return nil
 	}
-	var regularSrcs []string
+
-	for src := range srcMap {
-		regularSrcs = append(regularSrcs, src)
-	}
 	copyCmd := CopyCommand{
 		cmd: &instructions.CopyCommand{
-			SourcesAndDest: append(regularSrcs, dest),
+			SourcesAndDest: append(unresolvedSrcs, dest),
 		},
 		buildcontext: a.buildcontext,
 	}
@@ -117,19 +107,6 @@ func (a *AddCommand) ExecuteCommand(config *manifest.Schema2Config) error {
 	return nil
 }
 
-func isFilenameSource(srcMap map[string][]string, fileName string) (bool, error) {
-	for src := range srcMap {
-		matched, err := filepath.Match(src, fileName)
-		if err != nil {
-			return false, err
-		}
-		if matched || (src == fileName) {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 // FilesToSnapshot should return an empty array if still nil; no files were changed
 func (a *AddCommand) FilesToSnapshot() []string {
 	return a.snapshotFiles

pkg/commands/copy.go

@@ -17,14 +17,13 @@ limitations under the License.
 package commands
 
 import (
-	"os"
-	"path/filepath"
-	"strings"
-
 	"github.com/GoogleContainerTools/kaniko/pkg/util"
 	"github.com/containers/image/manifest"
 	"github.com/docker/docker/builder/dockerfile/instructions"
 	"github.com/sirupsen/logrus"
+	"os"
+	"path/filepath"
+	"strings"
 )
 
 type CopyCommand struct {
@@ -46,52 +45,42 @@ 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
-	srcMap, err := util.ResolveSources(resolvedEnvs, c.buildcontext)
+	srcs, err = util.ResolveSources(resolvedEnvs, c.buildcontext)
 	if err != nil {
 		return err
 	}
-	// For each source, iterate through each file within and copy it over
+	// For each source, iterate through and copy it over
-	for src, files := range srcMap {
+	for _, src := range srcs {
-		for _, file := range files {
+		fullPath := filepath.Join(c.buildcontext, src)
-			fi, err := os.Lstat(filepath.Join(c.buildcontext, file))
+		fi, err := os.Lstat(fullPath)
+		if err != nil {
+			return err
+		}
+		destPath, err := util.DestinationFilepath(src, dest, config.WorkingDir)
+		if err != nil {
+			return err
+		}
+		if fi.IsDir() {
+			if err := util.CopyDir(fullPath, dest); err != nil {
+				return err
+			}
+			copiedFiles, err := util.Files(dest)
 			if err != nil {
 				return err
 			}
-			destPath, err := util.DestinationFilepath(file, src, dest, config.WorkingDir, c.buildcontext)
+			c.snapshotFiles = append(c.snapshotFiles, copiedFiles...)
-			if err != nil {
+		} else if fi.Mode()&os.ModeSymlink != 0 {
+			// If file is a symlink, we want to create the same relative symlink
+			if err := util.CopySymlink(fullPath, destPath); err != nil {
 				return err
 			}
-			// If source file is a directory, we want to create a directory ...
+			c.snapshotFiles = append(c.snapshotFiles, destPath)
-			if fi.IsDir() {
+		} else {
-				logrus.Infof("Creating directory %s", destPath)
+			// ... Else, we want to copy over a file
-				if err := os.MkdirAll(destPath, fi.Mode()); err != nil {
+			if err := util.CopyFile(fullPath, destPath); err != nil {
-					return err
+				return err
-				}
-			} else if fi.Mode()&os.ModeSymlink != 0 {
-				// If file is a symlink, we want to create the same relative symlink
-				link, err := os.Readlink(filepath.Join(c.buildcontext, file))
-				if err != nil {
-					return err
-				}
-				linkDst := filepath.Join(destPath, link)
-				if err := os.Symlink(linkDst, destPath); err != nil {
-					logrus.Errorf("unable to symlink %s to %s", linkDst, destPath)
-					return err
-				}
-			} else {
-				// ... Else, we want to copy over a file
-				logrus.Infof("Copying file %s to %s", file, destPath)
-				srcFile, err := os.Open(filepath.Join(c.buildcontext, file))
-				if err != nil {
-					return err
-				}
-				defer srcFile.Close()
-				if err := util.CreateFile(destPath, srcFile, fi.Mode()); err != nil {
-					return err
-				}
 			}
-			// Append the destination file to the list of files that should be snapshotted later
 			c.snapshotFiles = append(c.snapshotFiles, destPath)
 		}
 	}

pkg/util/command_util.go

@@ -83,8 +83,8 @@ 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) (map[string][]string, error) {
+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
 	if ContainsWildcards(srcs) {
@@ -99,13 +99,8 @@ func ResolveSources(srcsAndDest instructions.SourcesAndDest, root string) (map[s
 		}
 		logrus.Debugf("Resolved sources to %v", srcs)
 	}
-	// Now, get a map of [src]:[files rooted at src]
-	srcMap, err := SourcesToFilesMap(srcs, root)
-	if err != nil {
-		return nil, err
-	}
 	// Check to make sure the sources are valid
-	return srcMap, IsSrcsValid(srcsAndDest, srcMap)
+	return srcs, IsSrcsValid(srcsAndDest, srcs, root)
 }
 
 // matchSources returns a list of sources that match wildcards
@@ -141,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
 		}
@@ -187,45 +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, srcMap map[string][]string) error {
-	srcs := srcsAndDest[:len(srcsAndDest)-1]
-	dest := srcsAndDest[len(srcsAndDest)-1]
-
-	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

@@ -210,3 +210,61 @@ func DownloadFileToDest(rawurl, dest string) error {
 	}
 	return os.Chtimes(dest, mTime, mTime)
 }
+
+// CopyDir copies the file or directory at src to dest
+func CopyDir(src, dest string) error {
+	files, err := RelativeFiles("", src)
+	if err != nil {
+		return err
+	}
+	for _, file := range files {
+		fullPath := filepath.Join(src, file)
+		fi, err := os.Stat(fullPath)
+		if err != nil {
+			return err
+		}
+		destPath := filepath.Join(dest, file)
+		if fi.IsDir() {
+			logrus.Infof("Creating directory %s", destPath)
+			if err := os.MkdirAll(destPath, fi.Mode()); err != nil {
+				return err
+			}
+		} else if fi.Mode()&os.ModeSymlink != 0 {
+			// If file is a symlink, we want to create the same relative symlink
+			if err := CopySymlink(fullPath, destPath); err != nil {
+				return err
+			}
+		} else {
+			// ... Else, we want to copy over a file
+			if err := CopyFile(fullPath, destPath); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// CopySymlink copies the symlink at src to dest
+func CopySymlink(src, dest string) error {
+	link, err := os.Readlink(src)
+	if err != nil {
+		return err
+	}
+	linkDst := filepath.Join(dest, link)
+	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 {
+		return err
+	}
+	logrus.Infof("Copying file %s to %s", src, dest)
+	srcFile, err := os.Open(src)
+	if err != nil {
+		return err
+	}
+	defer srcFile.Close()
+	return CreateFile(dest, srcFile, fi.Mode())
+}