-
Notifications
You must be signed in to change notification settings - Fork 108
feat: add support for supplying config options #45
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
Conversation
@@ -71,5 +72,7 @@ export class ConnectTool extends MongoDBToolBase { | |||
}); | |||
|
|||
this.mongodbState.serviceProvider = provider; | |||
this.state.connectionString = connectionString; | |||
await saveState(this.state); |
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 I understand correct that we'd save the conectionString with its credentials in a plaintext file this way?
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.
Yes - this should be handled in #20.
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.
question: why do we need mongodbState
and also state
? does mongodbState
get's persisted?
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.
No - mongodb state is a set of globals that will not get stored on disk. I was debating between adding them to state and then special-casing which fields get persisted but at the end opted to create a separate object.
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.
so far config is read only from env vars / defaults and state is something we expect to store (as in preferences).
I think we are merging these two concepts together in this PR, which I'm ok with.
In theory mongodbState
is mongodbConfig
then.
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.
Technically, it's not even a config - it's just a place to stash certain things we may need for later (such as the MongoDB client after connect
is invoked). This is technically session state as opposed to the current state, which is persistent across sessions. I'll probably clean it up in a separate PR.
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.
LGTM
@nirinchev are we merging state and config this way? in cli our approach has been one file locations are as follows
which seems to be the most common across cli tools I've seen |
* main: chore: refactor package.json import (#43)
Hm... good point - I thought the app directory could be convenient place to store it, but it's probably not that intuitive. Happy to change it to either the Atlas CLI or the mongosh locations. |
I think we should do
and replace |
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.
All of my comments are nits and I'm good to resolve, in fact we would be ok with just env vars and arguments for configuration
Okay, so for now we'll look for config in:
This almost matches mongosh (except for the fallbacks on macOS). Since I wanted to keep it simple for now and keep the config getting loaded synchronously, I'll keep it like that. We can (and should) definitely refactor it to load the config asynchronously at some point, in which case we can add the extra fallback locations. I'll look into the state situation at a follow-up PR. |
Adds the following mechanisms for supplying config (in order of precedence):
MDB_MCP_UPPER_SNAKE_CASE
mongodb-mcp.conf
placed in the/etc
folder on Unix or%LOCAL_APP_DATA%\mongodb\mongodb-mcp
.Fixes #21