Code review comment for lp:~natefinch/juju-core/034-juju-mongo

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM with a few suggestions below.

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go
File provider/local/prereqs.go (right):

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go#newcode94
provider/local/prereqs.go:94: " Juju requires version %v or greater.",
s/" Juju/"Juju/ - no need for the extra space in here where there's
another in the previous line.

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go#newcode113
provider/local/prereqs.go:113: return version.Zero, errors.New("Couldn't
parse version")
s/Couldn't parse version/Could not parse mongod version/ - slightly
better and clearer I think.

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs_test.go
File provider/local/prereqs_test.go (right):

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs_test.go#newcode129
provider/local/prereqs_test.go:129: c.Assert(err, gc.NotNil)
Check the actual error message?

https://codereview.appspot.com/66230043/diff/1/upstart/export_test.go
File upstart/export_test.go (right):

https://codereview.appspot.com/66230043/diff/1/upstart/export_test.go#newcode3
upstart/export_test.go:3: package upstart
Blank line before the package line. Also no need for 2013 in the
copyright.

https://codereview.appspot.com/66230043/diff/1/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/66230043/diff/1/upstart/service.go#newcode22
upstart/service.go:22: // MongoPath returns the executable path to be
used to run mongod on this machine.
// MongodPath ... (to match the func name). Also s/the executable//.
Actually, I'd prefer if you called this MongoDPath, so it matches the
SetMongoDPath from export_test.

https://codereview.appspot.com/66230043/diff/1/upstart/upstart_test.go
File upstart/upstart_test.go (right):

https://codereview.appspot.com/66230043/diff/1/upstart/upstart_test.go#newcode260
upstart/upstart_test.go:260: defer os.Remove(d)
os.RemoveAll(d) instead?

https://codereview.appspot.com/66230043/

« Back to merge proposal