-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update configuration schema for MongoDB adaptor. #354
Update configuration schema for MongoDB adaptor. #354
Conversation
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 looks great @bensonmiller, thanks!
We've got a lot going on here but we'll try and get this released today.
Hi @bensonmiller - I've been digging a bit deeper and I think I've found a couple of issues in the mongo adaptor. Bear with us while we get this sorted out! |
Hi @bensonmiller - we're going to merge and release this, but it looks like the mongo adaptor could use a little love :'( I've raised a bunch of issues - would you mind taking a quick look and letting me know if any of these are going to block you? I'm also interested if you have any other feedback, suggestions or insights.
Thanks! |
@josephjclark Thank you for the deep dive here on the MongoDB adaptor. None of these changes would block me. I would note that I'm not deeply experienced with MongoDB, so my current uses are relatively unsophisticated. Nevertheless, I've looked at each of the issues you opened and they all look reasonable to me. If I were to rank these issues according to my own interest/priority: HIGH: MED: LOW: |
@bensonmiller hey, thank you for going through this, that's so helpful! Nothing looks critical for right now so we'll get this released for you. (By the way, I've not forgotten the JSON schema stuff, I am still looking into it! There are some security concerns I'm exploring) |
Hi @bensonmiller, just to let you know that this should be live on Lightning. We did a major bump on the adaptor so you'll need to be using Mongo 2.0.0 (or latest) |
Add support to supplying a project id when pulling
Fixes #353
Summary
Fixes JSON schema verification for mongodb clusterUrl configuration
Details
In packages/mongodb/src/Adaptor.js, the connect() function constructs the URI for the MongoDB cluster as follows:
The adaptor expects the
clusterUrl
configuration property be provided as a hostname and not a full URI, but the current JSON schema specifies the format of this property asuri
, which fails validation when providing a normal hostname. This PR changes the JSON schema to verify this property as the correct format (hostname
, noturi
). The PR also renames the property and fixes all references to the property in the adaptor and unit tests.Issues
#353
Review Checklist
Before merging, the reviewer should check the following items:
migration guide followed?
dev only changes don't need a changeset.