Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade mongodb driver #472

Closed
wants to merge 22 commits into from
Closed

upgrade mongodb driver #472

wants to merge 22 commits into from

Conversation

naderio
Copy link

@naderio naderio commented Mar 31, 2018

  • upgrade mongodb driver to ^2.2.22 (to support MongoDB 3.6) and update connection options
  • cleanup/fix find() and destroy()
  • drops support for Node.js 0.10 since it breaks tests
  • add Node.js 8, 9 and 10 to CI

@mikermcneil
Copy link
Member

@naderio would you mind rebasing this to master?

@AtiX
Copy link

AtiX commented Jul 19, 2018

Any chance @naderio you could get this ready to be merged? I'd highly appreciate it!

@naderio
Copy link
Author

naderio commented Jul 25, 2018

@mikermcneil this PR is patch intended for version 0.12.x (branch 0.12.x) and NOT for 1.x (branch master) .. therefore rebasing against master is not needed
@AtiX PR is ready to be merged

CHANGELOG.md Outdated
@@ -1,5 +1,10 @@
# Sails-Mongo Changelog

### 0.12.4

* [INTERNAL] Bump `mongodb` version for

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is not finished

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrick-webs fixed, thanks

@patrick-webs
Copy link

@mikermcneil Is this likely/unlikely to be merged at some point and released as 0.12.4?

I'm investigating updating a sails 0.12 app to work with newer MongoDB versions and hoping to avoid having to upgrade to sails 1.x at the same time, another 0.12.x release would be great for us.

MongoDB 3.0 and 3.2 support has been EOLed as of this month and Mongo's driver documentation states that a driver >=2.2.12 is recommended to use MongoDB 3.4.

@mikermcneil
Copy link
Member

cc @luislobo

@luislobo
Copy link
Contributor

@patrick-webs @mikermcneil I actually have a fork with a branch that has updated mongodb drivers that work with Atlas (we are using it ourselves).
I can create a PR, and submit it.

And in the near future, I plan to evaluate migrating sails-mongo to the latest native mongodb driver. But at least the one I have works.
Check if our branch works for you https://github.com/PetroCloud/sails-mongo/tree/0-13

@luislobo
Copy link
Contributor

luislobo commented Sep 13, 2018

I will compare this PR and our branch to see if we can join efforts.

@naderio
Copy link
Author

naderio commented Sep 13, 2018

FYI I have this version running for months in production with MongoDB 3.6 and no issues.
There was a number of warnings that got removed after updating connection options.

@luislobo
Copy link
Contributor

@naderio yeah, my branch had the same issues. I'l double check mine and yours, and see if it is fully compatible with MongoDB Atlas

@patrick-webs
Copy link

FYI @naderio , I tried using your branch, it worked fine to connect to a local MongoDB 3.4 instance. When I tried connecting to a MongoDB 3.4 on Atlas, I began to get issues causing sails to not start:

MongoError: connection 8 to obfuscated-shard-00-02-dmqao.mongodb.net:27017 closed

I found that I had to add a ssl: true option to the config/connections.js file which had not been required before; my connection string already had ssl=true in it. It seems like something about the upgrade made that query string parameter in the connection string get ignored. I don't know if it's code in the branch, or if it was a change in the mongodb driver, but figured I'd document it here in case anyone else runs into the same issues.

@luislobo
Copy link
Contributor

@patrick-webs Yeah, we had to do the same here too. I'll try to get some time extra to review this PR during the weekend.

@luislobo
Copy link
Contributor

luislobo commented Nov 4, 2018

@naderio I've added several comments in the PR's code. To add to those, what @patrick-webs mentions should be added to the README.md too, as an "atlas" configuration section.

@naderio
Copy link
Author

naderio commented Nov 20, 2018

@luislobo @patrick-webs ssl issue should be fixed, tested with mongodb atlas

@constantx
Copy link

any chance for this to be merged soon?

@simonmilz
Copy link

I just received an email from mLab that they upgrade all mongo instances to 3.6 by May 7th. Any chance to get this PR merged soon?

@patrick-webs
Copy link

patrick-webs commented Apr 8, 2019

FWIW, I privately forked 97e6a4f and published it to a private npm module for my use, we've been using it in production with MongoDB Atlas without issue for the last 6 months.

@simonmilz
Copy link

@patrick-webs can you please share you implementation? The mentioned repo is private.

@simonmilz
Copy link

I applied the fixes here: https://github.com/TypischTouristik/sails-mongo

@patrick-webs
Copy link

patrick-webs commented Apr 12, 2019

@simonmilz Sorry, I referenced the wrong repo in my previous comment. I updated my comment to reference the commit from this pull request that I meant to reference. The fork I am using has no other changes besides private configuration files for building and uploading to my private npm repository.

@manuelzi
Copy link

I applied the fixes here: https://github.com/TypischTouristik/sails-mongo

Thanks for sharing this. Has anyone tried to upgrade the driver on the legacy branch to >=3.1 for MongoDB 4.0 support?

@simonmilz
Copy link

@manuelzi we are working on this right now.

@manuelzi
Copy link

@simonmilz Ok thanks, let me know if you need help testing.

@DominusKelvin
Copy link
Collaborator

Hey @naderio, @AtiX, @simonmilz, @manuelzi, @constantx, and, @patrick-webs, I'll be closing this PR as this other PR has implemented the upgrade and it has been merged. Thank you for working on this @naderio 💙

Also you can check here for the upgrade information. 👇🏾

https://blog.sailscasts.com/sails-mongo-v2-1-0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants