Code review comment for lp://staging/~wallyworld/goose/public-containers

Revision history for this message
Martin Packman (gz) wrote :

Changes LGTM, I've got a couple of suggested tweaks (including
re-joining the revision history of client/client.go), but will make
those myself and propose/land.

https://codereview.appspot.com/6962052/diff/9001/client/local_test.go
File client/local_test.go (right):

https://codereview.appspot.com/6962052/diff/9001/client/local_test.go#newcode47
client/local_test.go:47: + s.Server.URL, //public
Rather than relying on the order here and trusting the comments remain
in sync with the code, I'd prefer using the named field idiom, which
self-documents and is checked by the compiler:
     AdminURL: s.server.URL,
     InternalURL: s.server.URL,
     PublicURL: s.server.URL,

https://codereview.appspot.com/6962052/diff/9001/identity/legacy_test.go
File identity/legacy_test.go (right):

https://codereview.appspot.com/6962052/diff/9001/identity/legacy_test.go#newcode27
identity/legacy_test.go:27: + map[string]string{"compute":
"http://management/url/compute", "object-store":
"http://management/url/object-store"})
When making up urls for testing, I like using http://test.invalid/...
which you can trust will raise an error on dns lookup if the test
accidentally tries to resolve it.

https://codereview.appspot.com/6962052/diff/9001/swift/live_test.go
File swift/live_test.go (right):

https://codereview.appspot.com/6962052/diff/9001/swift/live_test.go#newcode208
swift/live_test.go:208: s.swift.DeleteObject(s.containerName, files[i])
Reasonable use of Check above to ensure the DeleteObject calls should
always happen, test might be clearer with comment to that effect though.

https://codereview.appspot.com/6962052/diff/9001/swift/swift.go
File swift/swift.go (left):

https://codereview.appspot.com/6962052/diff/9001/swift/swift.go#oldcode33
swift/swift.go:33: // with some refactoring, but for now just make
everything public.
This comment wants updating/removing now I'd say.

https://codereview.appspot.com/6962052/

« Back to merge proposal