fix mongo password authentication

[csanquer]

Hi,

With this PR, I fixed the mongo DB user/password authentication and authentication database parameter.

[j-abi]

@jaberg Could we include this simple fix in milestone https://github.com/hyperopt/hyperopt/milestone/3 ?

[jaberg]

I don't recall this is tested, how should it be tested? Does anyone have an idea how to set up Travis to do this? cc @maxpumperla @michaelmior

[jaberg]

I imagine this breaks compatibility with some old versions of Mongo, could you provide info on when Mongo started working this new way?

[michaelmior]

AFAICT, it's pymongo which has changed, not MongoDB. If that's the case, I don't think we need to worry about backwards compatibility. People who want to use a new release of hyperopt will just need to install a new pymongo.

[jaberg]

Someone reported recently that they tried running all tests, including mongo tests, and they all passed. Is it the case that no test performs authentication? Or maybe those tests were run with an old version of pymongo? How might we test authentication, see that this change works, and catch similar future API regressions?

…

On Wed, Feb 7, 2018 at 4:18 PM, Michael Mior ***@***.***> wrote: AFAICT, it's pymongo which has changed, not MongoDB. If that's the case, I don't think we need to worry about backwards compatibility. People who want to use a new release of hyperopt will just need to install a new pymongo. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#309 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKdDH-ZYJScTk4wQ3gMt75zgwt4kn_Uks5tShM8gaJpZM4NwIQU> .

[michaelmior]

I haven't looked at the tests currently configured, but if we have a test that uses authentication on Travis, this shouldn't be a problem.

[j-abi]

I don't think the current tests use any authentication. Actually, I think they do not use the MongoDB service as it is configured in .travis.yml, but (if I understand correctly) rather generate a temporary database locally: https://github.com/hyperopt/hyperopt/blob/master/hyperopt/tests/test_mongoexp.py#L56

As it is now, the Travis build succeeds after rebasing this merge request on the current master, even if we completely remove the MongoDB service in .travis.yml.

Personally, I feel like this is OK for the current release, as it adds authentication support while not breaking anything. Ideally, writing specific tests for MongoDB authentication would require a significant refactoring of test_mongo.exp in order to use the Travis CI service (probably using environment variables) rather than starting a new DB.

[jaberg]

Thanks for thinking about this @abiteboul I agree with you.

If Travis passes this, let's merge it.

[michaelmior]

This should probably be rebased on top of master. Tests are failing because the Travis config isn't in this branch.

[csanquer]

The branch has been rebased on top of master.

[michaelmior]

Thanks @csanquer! Merging.