search

db-migrate / mongodb

mongodb driver for db-migrate
Other
25 stars 58 forks source link

[WIP] Upgrade to mongodb 3.x.x #41

Closed ben833 closed 3 years ago

lgtm-com[bot] commented 5 years ago

This pull request introduces 1 alert when merging 248220e99e739a01554365c34a0446ef1f5083f1 into fdf2359d04b933630c3a99694c56b1d969eb7995 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 5 years ago

This pull request introduces 1 alert when merging ebd8c2f0cf74d84cb06d34518ae0b347f4fbe97f into fdf2359d04b933630c3a99694c56b1d969eb7995 - view on LGTM.com

new alerts:

wzrdtales commented 5 years ago

Thank you for your effort here! Please don't upgrade vows. All versions other than 0.8.0 which it is strictly fixed to are horribly buggy and causing tests to hang up and also show some other bugs. We decided to remove vows completely in favour of lab, unfortunately that did not happen for a few drivers yet.

commitlint-wzrdtales[bot] commented 4 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

BorntraegerMarc commented 4 years ago

Is there anything I can help you with so we can merge this PR?

wzrdtales commented 4 years ago

Hey @BorntraegerMarc Glad you ask. Since I am very rarely a mongodb user, it would be actually great to get help from someone that is using it more often. If you could test this PR for functionality and identify potential regressions should you have a project at hand where you could test against that would be tremendous help!

commitlint-wzrdtales[bot] commented 4 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

wzrdtales commented 4 years ago

and also the db.close issue mentioned in the thread earlier has to be taken care of.

commitlint-wzrdtales[bot] commented 4 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

wzrdtales commented 4 years ago

Just made sure the tests are running again on travis though

BorntraegerMarc commented 4 years ago

Sorry, I was swamped at work the last weeks. Looking into this now

BorntraegerMarc commented 4 years ago

Could someone tell me what exactly this PR wants to solve? Because it seems like on master we already use the v3 of mongodb: https://github.com/db-migrate/mongodb/blob/master/package.json#L30

ben833 commented 4 years ago

Could someone tell me what exactly this PR wants to solve? Because it seems like on master we already use the v3 of mongodb: https://github.com/db-migrate/mongodb/blob/master/package.json#L30

Yes, package.json is updated, but the tests fail. With the db.close() we get errors in tests, without it, the number of connections "balloon up" to 10+ during the test, which seems weird, but might be okay. I plan to test this on a real db migration soon.

BorntraegerMarc commented 4 years ago

Yes, package.json is updated, but the tests fail. With the db.close() we get errors in tests, without it, the number of connections "balloon up" to 10+ during the test, which seems weird, but might be okay. I plan to test this on a real db migration soon.

Ah, so this PR is to fix the tests 🙂 Thx for the help! I'll also start looking into it

ben833 commented 4 years ago

Yes, package.json is updated, but the tests fail. With the db.close() we get errors in tests, without it, the number of connections "balloon up" to 10+ during the test, which seems weird, but might be okay. I plan to test this on a real db migration soon.

Ah, so this PR is to fix the tests 🙂 Thx for the help! I'll also start looking into it

Well not just the tests -- the new version of mongo manages connections/sessions differently -- so we're looking into whether db.close is needed in index.js https://github.com/db-migrate/mongodb/pull/41/files#diff-168726dbe96b3ce427e7fedce31bb0bcL307

The node mongodb driver's changelog for 3.0.0 notes that the way mongo handles sessions is different, so we probably need to close the connection differently or not at all.

Version 3.6 of the server introduces the concept of logical sessions for clients. In this driver, MongoClient now tracks all sessions created on the client, and explicitly cleans them up upon client close. More information can be found in the Driver Sessions Specification.

https://github.com/mongodb/node-mongodb-native/blob/master/CHANGES_3.0.0.md

ben833 commented 3 years ago

Closing this, as I am not working on this, and don't want it to appear like I am working this