-
Notifications
You must be signed in to change notification settings - Fork 36
Add support for compiling on a remote docker #449
Conversation
✅ Hey viovanov! The commit authors and yourself have already signed the CLA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't actually tested the code streaming to a remote docker instance.
Most of the review feedback is stylistic only, except for the need to keep the support for symlinks in tar files in the vendored mholt/archiver
package.
Gopkg.toml
Outdated
@@ -71,7 +75,7 @@ | |||
|
|||
[[constraint]] | |||
name = "github.com/mholt/archiver" | |||
version = "2.1.0" | |||
version = "3.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change will make us drop a local patch to the vendor tree: https://github.com/cloudfoundry-incubator/fissile/pull/414/files
We have an open issue to update this dependency once the upstream PR has been merged: #416
I will need to rebase my PR on top of a restructuring of the code from v2 to v3, and then still need to add additional code for Windows support before we can expect this to be merged.
So for now we should not updated our vendored version of mholt/archiver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah.. I have to revert a bunch of code
the openshift package I used for streaming has symlink support, maybe we should consider switching if your PR is stuck
docker/docker.go
Outdated
// Stream files within the container just before starting | ||
fsWithSymlinks := fs.NewFileSystem() | ||
fsWithSymlinks.KeepSymlinks(true) | ||
for src, dst := range opts.StreamIn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dst
should be dest
. See also usage just 40 lines above.
docker/docker.go
Outdated
fsWithSymlinks := fs.NewFileSystem() | ||
fsWithSymlinks.KeepSymlinks(true) | ||
for src, dst := range opts.StreamIn { | ||
tarstream := tarstream.New(fsWithSymlinks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use tarStream
to avoid using the same name for a package and a variable?
docker/docker.go
Outdated
|
||
// Function for streaming files out of the container | ||
streamOutFiles := func() error { | ||
for src, dst := range opts.StreamOut { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dest
, see above.
docker/docker.go
Outdated
r, w := io.Pipe() | ||
|
||
go func() { | ||
downloadErr := d.client.DownloadFromContainer(container.ID, dockerclient.DownloadFromContainerOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err
?
docker/docker.go
Outdated
// we need to move things arround | ||
sourceDirectoryName := filepath.Base(src) | ||
streamedOutputDir := filepath.Join(dst, sourceDirectoryName) | ||
tmpDstDir := filepath.Join(filepath.Dir(dst), fmt.Sprintf("%s-tmp", filepath.Base(dst))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempDestDir
?
docker/docker.go
Outdated
@@ -605,11 +689,18 @@ func (d *ImageManager) RunInContainer(opts RunInContainerOpts) (exitCode int, co | |||
} else { | |||
exitCode = -1 | |||
} | |||
|
|||
// Stream files out of the container | |||
streamErr := streamOutFiles() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err
?
@@ -586,7 +671,15 @@ func (d *ImageManager) RunInContainer(opts RunInContainerOpts) (exitCode int, co | |||
closeFiles() | |||
if err != nil { | |||
exitCode = -1 | |||
return exitCode, container, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return exitCode, container, err | |
return -1, container, err |
and delete the assignment above, for consistency with other usage in the same function.
No description provided.