Skip to content

Upgrade mongodb to 4.8.1 (min version supporting mongo 6) #497

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Upgrade mongodb to 4.8.1 (min version supporting mongo 6) #497

wants to merge 1 commit into from

Conversation

@vidueirof
Copy link

Tested locally with mongo 4, 5 and 6. Also with Atlas (free tier).

@sailsbot
Copy link
Collaborator

Thanks for submitting this pull request, @vidueirof! We'll look at it ASAP.

In the mean time, here are some ways you can help speed things along:

  • discuss this pull request with other contributors and get their feedback. (Reactions and comments can help us make better decisions, anticipate compatibility problems, and prevent bugs.)
  • ask another JavaScript developer to review the files changed in this pull request. (Peer reviews definitely don't guarantee perfection, but they help catch mistakes and enourage collaborative thinking. Code reviews are so useful that some open source projects require a minimum number of reviews before even considering a merge!)
  • if appropriate, ask your business to sponsor your pull request. (Open source is our passion, and our core maintainers volunteer many of their nights and weekends working on Sails. But you only get so many nights and weekends in life, and stuff gets done a lot faster when you can work on it during normal daylight hours.)
  • make sure you've answered the "why?" (Before we can review and merge a pull request, we feel it is important to fully understand the use case: the human reason these changes are important for you, your team, or your organization.)

Please remember: never post in a public forum if you believe you've found a genuine security vulnerability. Instead, disclose it responsibly.

For help with questions about Sails, click here.

@vidueirof
Copy link
Author

vidueirof commented Apr 18, 2023 •
edited
Loading

Ok, looking into AppVeyor error

@vidueirof
Copy link
Author

Mongo node driver 4.x and superior it's not compatible with node 10. More info: https://www.mongodb.com/docs/drivers/node/current/compatibility/#language-compatibility
Node 20 was released, don't you think it's time to deprecate node 10 support?

@vidueirof vidueirof changed the title Upgrade mondodb to 4.8.1 (min version supporting mongo 6) Upgrade mongodb to 4.8.1 (min version supporting mongo 6) Apr 18, 2023
@vidueirof vidueirof mentioned this pull request Apr 18, 2023
Closed
@luislobo
Copy link
Contributor

luislobo commented May 1, 2023

I have a PR that updates travis and replaces it with Github actions, using newer mongodb versions. I could add other node versions and MongoDB 6

#495

@mikermcneil What do you think about deprecating non-LTS node versions?

@vidueirof
Copy link
Author

At this time node 10 was left behind a long time ago. I think it's time to move forward.
Take a look at current supported versions:
schedule

@luislobo
Copy link
Contributor

luislobo commented May 2, 2023

I think we should get my PR approved first, so that we have a working CI, and then you can update it with the newer mongdb versions.

eashaw
eashaw requested changes May 5, 2023
View reviewed changes
lib/private/machines/count-records.js
var mongoCollection = db.collection(tableName);
mongoCollection.find(mongoWhere).count(function countCb(err, nativeResult) {
mongoCollection.countDocuments(mongoWhere, function countCb(err, nativeResult) {
//mongoCollection.find(mongoWhere).count(function countCb(err, nativeResult) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this comment?

lib/private/machines/create-each-record.js
_.each(s3q.newRecords, function (record){
record.id = nativeResult.insertedIds[index];
++index;
});
Copy link
Member

Choose a reason for hiding this comment

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

Is it normally _id?

lib/private/machines/create-record.js
// ============================================

s3q.newRecord.id = nativeResult.insertedId;
nativeResult.ops = [s3q.newRecord];
Copy link
Member

Choose a reason for hiding this comment

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

Mike: Hmmm, why mutate nativeResult?
Should this be _id?

@DominusKelvin
Copy link
Collaborator

Hey @vidueirof, 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 💙

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

Reviewers

@eashaw eashaw eashaw requested changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vidueirof @sailsbot @luislobo @DominusKelvin @eashaw