Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Fixes for replicaset #117
Conversation
macgreagoir
added some commits
Nov 8, 2016
| @@ -248,7 +248,7 @@ func (inst *MgoInstance) run() error { | ||
| return err | ||
| } | ||
| logger.Debugf("found mongod at: %q", mongopath) | ||
| - if mongopath == "/usr/lib/juju/bin/mongod" { | ||
| + if mongopath == "/usr/lib/juju/bin/mongod" || mongopath == "/usr/lib/juju/mongo3.2/bin/mongod" { |
frobware
Nov 23, 2016
This seems incredibly fragile. What happens for 3.3? Do we have to release a new version of juju?
jameinel
Nov 23, 2016
Owner
Given the differences in 2.4 vs 2.6 we weren't able to support them generically even with simple version changes. So we decided to explicitly version the mongo directory, so that new versions of Juju don't accidentally get a version of Mongo that they can't actually support.
Since we have to do work to enable Mongo anyway, we'd have to release new versions to get new Mongo. (btw, Mongo uses even/odd for stable/unstable, so we'll never do 3.3, but certainly we should be aware of what 3.4 might bring.)
| +func (inst *MgoInstance) MustDial() *mgo.Session { | ||
| + s, err := mgo.DialWithInfo(inst.DialInfo()) | ||
| + if err != nil { | ||
| + panic(err) |
jameinel
Nov 23, 2016
Owner
It is generally standard practice for a "MustX" function to panic if it cannot do X. I don't know why we need MustDial, rather than actually handling the error. As this is Testing infrastructure, I suppose it is ok, but I will note that failing an assertion is often much easier to debug than a panic.
jameinel
approved these changes
Nov 23, 2016
changes seem fine to me. they are a bit unfortunate, but not incorrect.
| +func (inst *MgoInstance) MustDial() *mgo.Session { | ||
| + s, err := mgo.DialWithInfo(inst.DialInfo()) | ||
| + if err != nil { | ||
| + panic(err) |
jameinel
Nov 23, 2016
Owner
It is generally standard practice for a "MustX" function to panic if it cannot do X. I don't know why we need MustDial, rather than actually handling the error. As this is Testing infrastructure, I suppose it is ok, but I will note that failing an assertion is often much easier to debug than a panic.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-testing |
macgreagoir commentedNov 22, 2016
replicaset tests seem not to have been passing for some time, and require these changes in mgo.go: