gfr10598
commented
7 years ago Review status: 0 of 8 files reviewed at latest revision, 13 unresolved discussions.
embargo.go, line 44 at r4 (raw file):
// Split one tar files into 2 buffers. func SplitFile(content io.Reader) (bool, bytes.Buffer, bytes.Buffer) {
Return an Error instead of a bool, and make it the last return value, consistent with other Go functions?
embargo.go, line 45 at r4 (raw file):
// Split one tar files into 2 buffers. func SplitFile(content io.Reader) (bool, bytes.Buffer, bytes.Buffer) { var privateBuf bytes.Buffer
Maybe call this "embargoBuf"?
embargo.go, line 54 at r4 (raw file):
} defer zipReader.Close() unzippedImage, err := ioutil.ReadAll(zipReader)
Maybe unzippedBytes ?
embargo.go, line 67 at r4 (raw file):
publicTw := tar.NewWriter(publicGzw) // Handle one tar file
This comment is confusing. Is it handling one inner file from the tar file, or a top level tar file, or something else?
embargo.go, line 88 at r4 (raw file):
// put this file to a private buffer if err := privateTw.WriteHeader(hdr); err != nil { fmt.Println(err)
This prints to the console. Is that what you want here? Or should it go to a log?
embargo.go, line 109 at r4 (raw file):
if err := publicTw.Close(); err != nil { fmt.Println(err) return false, privateBuf, publicBuf
In general, I think it is much better to return the error, and decide what to do with it in the caller. This makes testing easier and more robust, and gives the caller more control.
embargo.go, line 140 at r4 (raw file):
// Embargo do embargo ckecking to all files in the sourceBucket. func Embargo() bool {
I think we need a TODO here. If the embargo job crashes half way through, what should happen when it starts up again?
embargo_test.go, line 5 at r4 (raw file):
import ( "bytes" "fmt"
Run gofmt?
embargo_test.go, line 12 at r4 (raw file):
/* // End to end test, requires authentication.
Please add a clear TODO() here.
embargo_test.go, line 25 at r4 (raw file):
*/ func TestSplitTarFile(t *testing.T) {
I can figure out from context what is in the test files, but it would be better to state it explicitly in some combination of comment describing the test, and inline comments. If you define const strings for the filenames, the comments will be easy to read. E.g. "combined_file contains ... and ... . The test verifies that SplitFile correctly separates this into ... and ..., which ..."
An alternative (I think cleaner) way to test this would be to create a tar file on the fly, with lists of inner files, call SplitFile on it, then verify that the pub and private buffers contain the correct filenames.
embargo_test.go, line 39 at r4 (raw file):
t.Error("Did not perform embargo ocrrectly.\n") } publicGolden, err := os.Open("testdata/20170315T000000Z-mlab3-sea03-sidestream-0000-p.tgz")
Does this need
embargo_test.go, line 44 at r4 (raw file):
} defer publicGolden.Close() publicContent, err := ioutil.ReadAll(publicGolden)
Either check err value, or use _
embargo_test.go, line 56 at r4 (raw file):
} defer privateGolden.Close() privateContent, err := ioutil.ReadAll(privateGolden)
Check err or use _
Comments from Reviewable
yachang
Embargo cls, the third: do the tar file processing, split into 2 and save at different places.
This change is