chore: add arg-parser and put the config under test MCP-86 #429
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the configuration management system to use the @mongosh/arg-parser library for generating MongoDB connection strings. This change aligns with how mongosh handles connection arguments and ensures support for additional authentication mechanisms. The refactoring removes several deprecated flags and types that are no longer necessary with the consolidated approach.
- Uses mongosh argument parser to standardize connection string generation and authentication support
- Consolidates configuration by removing the separate
ConnectOptionsinterface and moving connection options to driver-level configuration - Removes hardcoded connection options from test files and connection calls throughout the codebase
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config.ts | Major refactor to use mongosh arg-parser with new configuration parsing and driver options setup |
| tests/unit/common/config.test.ts | Comprehensive test suite for the new configuration parsing functionality |
| src/index.ts | Added FIPS mode support, help/version modes, and system CA loading |
| tests/unit/common/session.test.ts | Removed deprecated connectOptions usage in tests |
| tests/integration/tools/mongodb/connect/connect.test.ts | Removed deprecated connectOptions usage in integration tests |
| tests/integration/common/connectionManager.test.ts | Removed deprecated connectOptions usage and import |
| src/tools/mongodb/mongodbTool.ts | Simplified connection call by removing connectOptions spread |
| src/tools/atlas/connect/connectCluster.ts | Simplified connection call by removing connectOptions spread |
| src/server.ts | Simplified connection call by removing connectOptions spread |
| src/resources/common/config.ts | Updated to use new driverOptions export instead of connectOptions |
| src/lib.ts | Removed ConnectOptions from exports |
| src/common/connectionManager.ts | Refactored to use new driverOptions configuration |
| package.json | Added @mongosh/arg-parser dependency |
Pull Request Test Coverage Report for Build 16878144687Details
💛 - Coveralls |
| @@ -1,11 +1,37 @@ | |||
| #!/usr/bin/env node | |||
|
|
|||
| let fipsError: Error | undefined; | |||
There was a problem hiding this comment.
why not just throw the error right away? would be good to avoid global variables if we can
There was a problem hiding this comment.
Because in line 108 we do more checks that need to be done after this.
- We first enable FIPS before anything.
- Then after we load the flags, we check different configurations and then give a specific error message (for example, shared openssl vs bundled)
- Then we show FIPS error details based on the actual configuration.
There was a problem hiding this comment.
Because in line 108 we do more checks that need to be done after this.
if we error in line 13, would we need to do any additional checks? it erroring then eventually leads to it being handled late with:
if (!fipsError && !crypto.getFips()) {
fipsError = new Error("FIPS mode not enabled despite requested.");
}
if config.tlsFIPSMode can be set to true in other ways other than argv then I don't think we'd support the alternative ways this way. If that's intended then that's fine
There was a problem hiding this comment.
The relevant info is in the if starting at line 115. If you see, we access process.config.variables.node_shared_openssl. This is not really relevant for us now: https://nodejs.org/api/process.html#processconfig
But I would like to keep this as close to how mongosh does it because if at some point we build our own snapshots (similar to what mongosh does) for reliability or easiness of distribution, this will be relevant.
I don't have a strong opinion either way, if you have it I can just change it, merge everything into enableFipsIfRequested and remove the assertFips function.
There was a problem hiding this comment.
if process.config.variables.node_shared_openssl is already set at this point it'd be nice to just have one function and no new globals
This commit is biggish because it adds a big chunk of tests that weren't there before. While it would be arguably that testing every flag might be too much, because I'm changing how arguments are parsed, I want to have this as a safeline.
For help we are refering to the README.md, if we want something more fancy we porobably want to have better documentation generation so we don't maintain two files (here and the README.md)
The driver options can not be sent by the agent, so we are going to use the ones specified in the CLI arguments / ENV Vars. This is relevant because new connections should use any inherited configuration like TLS settings or FIPS by default.
This is necessary when connecting for the first time.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Proposed changes
Use mongosh argument parser to generate the connection string that is going to be used to connect to MongoDB, aligning with how mongosh does it and making sure we support additional authentication mechanisms. It removes a bunch of flags and types not necessary now with the consolidation.
There will be integration tests for each of the supported connection types when playing each of their tasks. Specific support is out of the scope of this PR.
Checklist