Receive multiple dests #135
Conversation
Signed-off-by: a-palchikov <deemok@gmail.com>
…tay closer to the original and make it easier to grok Signed-off-by: a-palchikov <deemok@gmail.com>
Signed-off-by: a-palchikov <deemok@gmail.com>
| wcs []io.WriteCloser | ||
| } | ||
|
|
||
| func newMultiErr(errs ...error) error { |
There was a problem hiding this comment.
I did not attempt to introduce another dependency on something like go-multierror so this is ad-hoc multiple errors wrapper.
| @@ -158,17 +158,16 @@ func TestWalkerWriterSimple(t *testing.T) { | |||
| err = Walk(context.Background(), dest, nil, bufWalk(b)) | |||
| assert.NoError(t, err) | |||
|
|
|||
| assert.Equal(t, string(b.Bytes()), `dir bar | |||
| assert.Equal(t, `dir bar | |||
There was a problem hiding this comment.
Swapped the arguments to match the error message better - expected is the first followed by actual. Otherwise, the error message on failure is confusing.
| @@ -26,15 +26,18 @@ type ReceiveOpt struct { | |||
| Merge bool | |||
| Filter FilterFunc | |||
| Differ DiffType | |||
|
|
|||
| diskWriterOpt DiskWriterOpt | |||
There was a problem hiding this comment.
added explicit configuration attribute to work around the necessity to run the test as root (see below).
| @@ -67,8 +67,6 @@ func TestInvalidExcludePatterns(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestCopyWithSubDir(t *testing.T) { | |||
| requiresRoot(t) | |||
There was a problem hiding this comment.
Instead of running as root, the test records changes to the file metadata.
| @@ -178,7 +240,7 @@ func TestCopySimple(t *testing.T) { | |||
| "ADD zzz/bb dir", | |||
| "ADD zzz/bb/cc dir", | |||
| "ADD zzz/bb/cc/dd symlink ../../", | |||
| "ADD zzz.aa zzdata", | |||
| "ADD zzz.aa file zzdata", | |||
There was a problem hiding this comment.
This was confusing - was the omission of the file type intended? Let me know if this is the case and how to interpret the test then. I had to add the file type as I updated the changes language parser to panic on unexpected input.
| @@ -112,10 +174,10 @@ func TestCopySwitchDirToFile(t *testing.T) { | |||
|
|
|||
| dest, err := tmpDir(changeStream([]string{ | |||
| "ADD foo dir", | |||
| "ADD foo/bar dile data2", | |||
| "ADD foo/bar file data2", | |||
There was a problem hiding this comment.
Was this a typo or intended?
| @@ -231,6 +231,8 @@ func parseChange(str string) *change { | |||
| } | |||
| st.Mode |= uint32(os.ModeSymlink) | |||
| st.Linkname = f[3] | |||
| default: | |||
| panic(fmt.Sprint("unrecognized file type:", typ)) | |||
There was a problem hiding this comment.
Added this to find suspected typos in input (see above). If this is intended - please let me know.
| } | ||
|
|
||
| func Receive(ctx context.Context, conn Stream, dest string, opt ReceiveOpt) error { | ||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
This looked like a no-op - was it intended?
There was a problem hiding this comment.
Do you have a good use-case in mind for this? It seems quite complicated and I don't consider this aspect a blocker for moby/buildkit#2760 . Or I'm also fine if this edge case is handled non-optimally by just transferring things twice.
| if r.merge { | ||
| err = createDir(ctx, dw.HandleChange, w.fill) | ||
| } else { | ||
| err = doubleWalkDiff(ctx, dw.HandleChange, getWalkerFn(r.dests[0]), w.fill, r.filter, r.differ) |
There was a problem hiding this comment.
Not sure how this works if the comparison for the differ is always based on the first path?
|
I have to agree to complexity. I will rewrite it as a corner case of copying twice (or however many times) which I was hesitating to implement. Thanks for your feedback! |
This PR implements support for syncing to multiple directories for moby/buildkit#2760 as needed when multiple output destinations of type
localhave been specified on thebuildctlcommand line.I'm open to suggestions on how to change/simplify this approach but that's the minimum I came up with.