-
Notifications
You must be signed in to change notification settings - Fork 260
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
[feat]: upgrade to mongodb driver 6 #499
[feat]: upgrade to mongodb driver 6 #499
Conversation
Thanks for submitting this pull request, @DominusKelvin! We'll look at it ASAP. In the mean time, here are some ways you can help speed things along:
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. |
Happy to work on this with you. Do you have any current task to delegate? Or if you have series of steps to follow so I can hop on one of them. I ask this in order to avoid future merge conflicts |
@DominusKelvin I've been spending a lot of time working on first cleaning up sails-mongo code to more current one, removing travis and appveyor and using github actions. We have been talking with @Josebaseba (who has been a long time maintainer/contributor, like me) to We are all wasting time working on the same things in parallel. Please check: #498 @mikermcneil We really need to get together and have a real action plan here. |
@DominusKelvin The goals is: once that PR is approved, it would be very simple to just migrate to 6 and have good code coverage in mongodb versions for both Linux and Windows as CI covers all combinations. |
Hey @luislobo great work on the PR you are working on I wish we've seen it earlier but I guess it sort of fall through the crack for us. I notice a couple of things like that you've changed like updating var to let let/const. Really cool Looking at something like create-record I think it's not quite trivial To move forward I think you, myself and, any other MongoDB user can see how to move forward on this as there are quite a lot of breaking changes we've got to consider |
Also @luislobo and @Josebaseba if you want, we can have a Discord call in the Sailscasts community to talk about this and moving forward. From the core team meeting last Thursday, @mikermcneil and myself have discussed prioritizing moving Let me know what you think. Here is a link to the community |
So one blocker I noticed(so far) is So the options we have is to do a |
So I have moved |
I can join in about an hour or so. I was already on SailsCast discord, just not active daily. |
Thanks, I'd love to do pair review, merge and then work on the other things. This PR changes code everywhere since it changes
It is my understanding that current Waterline version works also like that, unless you chain it with fetch.
I'm down to it. |
Should only be done if "fetch" is called in a chain https://sailsjs.com/documentation/reference/waterline-orm/models/create#?result |
BTW, to note: my PR also switches to GitHub Actions, removing AppVeyor (which is failing here) and Travis. And in the process, I've added test for MongoDB 4.4 (which is still LTS until Feb next year), 5.x, 6.x and 7.x |
Got you @luislobo i think for now we can prioritize moving the codebase to support v6 of the adapter and also change let/const for the files we touch plus the infra things you mentioned can be added as well. If you want we can pair right now to try fix some things I've noticed Pair on Discord? |
What's your handle so I ping you and we hop on a call together |
I'm back. I think I'm |
@@ -105,15 +106,19 @@ module.exports = { | |||
// ╠═╝╠╦╝║ ║║ ║╣ ╚═╗╚═╗ │││├─┤ │ │└┐┌┘├┤ ├┬┘├┤ │ │ │├┬┘ │││ └─┐ │ | |||
// ╩ ╩╚═╚═╝╚═╝╚═╝╚═╝╚═╝ ┘└┘┴ ┴ ┴ ┴ └┘ └─┘ ┴└─└─┘└─┘└─┘┴└──┴┘└─└─┘─┘ | |||
// Process record(s) (mutate in-place) to wash away adapter-specific eccentricities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ This comment makes more sense down below above the try/catch when we process records.
And instead, before that, we need a new comment explaining what we're doing in terms of fetching these records.
Minor thing, but I misunderstood when first reading so others are likely to as well (this'll prevent future confusion)
// Use unified topology. MongoDB node maintainers recommends this to be enabled | ||
// https://github.com/mongodb/node-mongodb-native/releases/tag/v3.2.1 | ||
// Use new url parser to remove warnings | ||
_clientConfig = Object.assign({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could be related to the ?
error @eashaw mentioned (no real reason why I'm suggesting that, other than at a glance, I'm noticing useNewUrlParser
, so maybe related?)
try { | ||
processNativeRecord(phRecord, WLModel, s3q.meta); | ||
} catch (e) { return exits.error(e); } | ||
(function(iifeDone) { mongoCollection.findOne({ _id: nativeResult.insertedId }).then(function(phRecord) { iifeDone(undefined, phRecord);}).catch(function(err) { iifeDone(err);});})(function(err, phRecord) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the useNewUrlParser @mikermcneil that's no longer a supported config option in the newer version of the MongoDB driver. @eashaw can you confirm if that's the case for you?
@@ -97,6 +97,7 @@ module.exports = { | |||
return exits.success(); | |||
}//-• | |||
|
|||
var insertedIds = Object.values(nativeResult.insertedIds); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should paste in (and slightly tweak) the assertion from here: https://github.com/balderdashy/sails-mongo/pull/499/files#diff-962fecaf7496f0dc0f4c49842e1e52f7035018902cd4530b86833883185c0166L106
that way we'll know if something changes in Mongo in the future and keep things just as easy to follow for migrating to future mongo driver releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right @mikermcneil. I just added the assertions and tweaked it to match what we are checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me other than comments above.
@eashaw is looking at a failing test (and a second look at the create-manager
function re: compatibility), but then assuming he doesn't run into anything else, I think this is ready to merge!
Thanks @DominusKelvin!
Thanks @mikermcneil. I think @eashaw has made a pass with the suggestions you made. @eashaw does the test still fails for you after you've made the changes? If not then I think we are definitely at the end of getting this merged. Thanks @eashaw for the changes made. |
I am glad it is :) |
Great job working on the CI passing @eashaw 🥳 |
Shouldn't the tests MongoDB servers be updated? The MongoDB server versions being tested on are EOL. 3.6.18: EOL April 2021 https://www.mongodb.com/support-policy/lifecycles I suggest using:
|
For the Travis version, the CI will have to be updated to a Links to download files are here: I can help with this update if you want to go on this direction for this PR. |
Hey @luislobo great suggestions. I think we can do the CI upgrades on a follow-up PR after this one. |
Hey all, just to state the obvious 😁, this PR has been approved for merge and a new version will be cut out soon. Thank you all for your patience on this one :) |
Hey there Sailors, I am pleased to announced that this PR has been merged and a minor release of sails-mongo - v2.1.0 has been made. Thank you all for your patience and do enjoy your latest sails-mongo adapter with the latest MongoDB driver. You can read the release blog post for more information on upgrading to this new version of sails-mongo 👇🏾 |
This PR is now ready for merging
This is gonna be minor release of
sails-mongo
i.ev2.1.0
I am adding this so the broader Sails community using MongoDB can pitch in
This PR when landed will close issue #7275 on the sails repo