-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: improve dependency handling and use the DataGrip connector to MongoDB #6
Conversation
…will use datagrip to query)
maxParameterListSize: '10' | ||
- name: FILE_NAME_INCORRECT | ||
enabled: false |
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.
Do we want to add comments to the YAML about why we're adding new rules here? That might not be easy to figure out in the future based on the file contents alone
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.
Added a comment for each rule explaining why it's disabled / enabled.
) = withContext(Dispatchers.IO) { | ||
runQuery( | ||
"""db.getSiblingDB("${namespace.database}") | ||
.getCollection("${namespace.collection}") |
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.
Do you want to account for escaping here? (This might already be addressed by your updates locally, but just checking)
Otherwise some kind of TODO might be nice, even if this is still WIP :)
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 added some escaping now, thanks for pointing out! I was escaping only when converting a string to a namespace using the .toNs
method, but not in the namespace constructor. Now I fixed it and it should be safe.
|
||
override fun beforeAll(context: ExtensionContext?) { | ||
val annotation = context!!.requiredTestClass.getAnnotation(IntegrationTest::class.java) | ||
val container = MongoDBContainer("mongo:${annotation.mongodb.versionString}-jammy") |
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.
Just a clarifying question: container usage = We're not planning to run tests on platforms other than Linux?
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'm not sure I follow, they should run on any environment with Docker/Podman (I'm running them on my Mac with Podman right now). Do you see any concern here?
Coverage Report
Files
|
Description
Checklist
Open Questions