use juju-mongod if it exists

use juju-mongod if it exists



https://code.launchpad.net/~natefinch/juju-core/034-juju-mongo/+merge/207320

(do not edit description out of merge proposal)

[natefinch, 2014-02-19 22:13:22]

Please take a look.

[fwereade, 2014-02-20 10:03:15]

I think this looks good (modulo needing to actually install juju-mongodb, bu I
understand that's a distinct CL), but I'd like another pair of eyes too please.

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

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go#oldc...
provider/local/prereqs.go:90: // TODO(axw) verify version/SSL capabilities
Remind me: what's the latest story on SSL support? did we finally get it
compiled into all the mongos we might use?

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#newcode31
upstart/service.go:31: return "mongod"
I has a nervous about changes. Does life get any better if we use the result of
`which mongod`?

https://codereview.appspot.com/66230043/diff/1/upstart/service.go#newcode32
upstart/service.go:32: }
Also, I remain twitchy about mixing the juju-specific bits into the
supposedly-generic upstart package. Is this a simple enough CL that we could
move them out?

[natefinch, 2014-02-20 12:33:48]

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

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go#oldc...
provider/local/prereqs.go:90: // TODO(axw) verify version/SSL capabilities
On 2014/02/20 10:03:15, fwereade wrote:
> Remind me: what's the latest story on SSL support? did we finally get it
> compiled into all the mongos we might use?

Good question, I don't know.

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#newcode31
upstart/service.go:31: return "mongod"
On 2014/02/20 10:03:15, fwereade wrote:
> I has a nervous about changes. Does life get any better if we use the result
of
> `which mongod`?

That's what I wrote first, but the problem is, many tests overwrite $PATH in
order to isolate themselves, and then this code can't find "which" to run it.  I
suppose I could hardcode the location of "which" to /usr/bin/which ... but I
figured the only diffference between running the results of which mongod and
just running "mongod" is that the former just hardcodes whatever mongod existed
at the time, and the latter uses whatever is first in the path when upstart
runs.  I don't know which is worse.

https://codereview.appspot.com/66230043/diff/1/upstart/service.go#newcode32
upstart/service.go:32: }
On 2014/02/20 10:03:15, fwereade wrote:
> Also, I remain twitchy about mixing the juju-specific bits into the
> supposedly-generic upstart package. Is this a simple enough CL that we could
> move them out?

Absolutely.  I actually put them here on purpose so someone would complain and
tell me where a better place would be.  Keep in mind this method is called both
by upstart and the local provider.

[dimitern, 2014-02-20 16:41:56]

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#newc...
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#newc...
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...
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#newcod...
upstart/upstart_test.go:260: defer os.Remove(d)
os.RemoveAll(d) instead?

[natefinch, 2014-02-21 13:46:23]

Please take a look.

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#newc...
provider/local/prereqs.go:94: " Juju requires version %v or greater.",
On 2014/02/20 16:41:56, dimitern wrote:
> s/" Juju/"Juju/ - no need for the extra space in here where there's another in
> the previous line.

Done.

https://codereview.appspot.com/66230043/diff/1/provider/local/prereqs.go#newc...
provider/local/prereqs.go:113: return version.Zero, errors.New("Couldn't parse
version")
On 2014/02/20 16:41:56, dimitern wrote:
> s/Couldn't parse version/Could not parse mongod version/ - slightly better and
> clearer I think.

Done.

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...
provider/local/prereqs_test.go:129: c.Assert(err, gc.NotNil)
On 2014/02/20 16:41:56, dimitern wrote:
> Check the actual error message?

Done.

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.
On 2014/02/20 16:41:56, dimitern wrote:
> // 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.

I removed the SetMongoDPath function since I needed to be able to set it from
outside the package too (for local provider tests)..... and I think the capital
d was a mistake anyway. The word should be all lowercase, except for the first
letter to make it public.