Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADD command #58

Merged
merged 6 commits into from
Apr 4, 2018
Merged

ADD command #58

merged 6 commits into from
Apr 4, 2018

Conversation

priyawadhwa
Copy link
Collaborator

The add command first resolves any environment variables and wildcards in the list of sources. It then passes through each source, unpacking it if it's a local tar archive or downloading it if it's a remote file URL. It removes these sources from the list of sources, and with any remaining "normal" sources it executes a standard copy command.

Fixes #3

Partially fixes #43

@@ -160,10 +170,31 @@ func DestinationFilepath(filename, srcName, dest, cwd, buildcontext string) (str
return filepath.Join(cwd, dest), nil
}

// URLDestinationFilepath gives the destintion a file from a remote URL should be saved to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: destination

return err
}
if util.IsSrcRemoteFileURL(file) {
urlDest := util.URLDestinationFilepath(file, dest, config.WorkingDir)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Might be worth splitting these three cases into 3 helper functions to shorten ExecuteCommand a bit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need access to the file, srcMap, destination, and config I figured it would make more sense to keep it in this function, instead of passing all those in as arguments

@@ -22,6 +22,8 @@ import (
"testing"
)

var testUrl = "https://github.com/GoogleCloudPlatform/runtimes-common/blob/master/LICENSE"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might make more sense to depend on something in k8s-container-builder but this is probably fine

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ideally the tests would not require pinging an actual URL, you can mock the response and retrieve the file that way. not necessary though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah I'd have liked to use something in this repo as well but it's still private

@@ -86,3 +92,83 @@ func checkHardlink(p string, i os.FileInfo) (bool, string) {
}
return hardlink, linkDst
}

//UnpackLocalTarArchive unpacks the tar archive at path to the directory dest
// Returns true if the path was acutally unpacked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits: Space after // for comment, actually

@priyawadhwa priyawadhwa merged commit ec44f96 into GoogleContainerTools:master Apr 4, 2018
@priyawadhwa priyawadhwa deleted the add branch April 4, 2018 21:32
@priyawadhwa priyawadhwa added the area/dockerfile-command For all bugs related to dockerfile file commands label Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dockerfile-command For all bugs related to dockerfile file commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support environment replacement Implement ADD command
3 participants