-
Notifications
You must be signed in to change notification settings - Fork 112
feat: add more details about atlas connect flow - MCP-124 #500
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
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.
Pull Request Overview
This PR adds more informative messaging to the Atlas connect flow to better inform users about resources that are automatically created during cluster connection. The changes track whether an IP access list entry or user was created and provide appropriate feedback messages.
- Modify
ensureCurrentIpInAccessListto return a boolean indicating if a new IP access list entry was created - Update the connect cluster flow to track user creation and provide detailed feedback about created resources
- Add informative messages about IP access list updates and temporary user creation with a reference to documentation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/common/atlas/accessListUtils.ts | Modified to return boolean indicating if new IP access list entry was created |
| src/tools/atlas/connect/connectCluster.ts | Enhanced to track resource creation and provide detailed user feedback messages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Pull Request Test Coverage Report for Build 17433820852Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Minor nits/questions, otherwise lgtm.
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.
It seems like we're always returning true for userCreated - am I missing something?
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, if the connection state doesn't require preparation (e.g. is already connected*) then we'd not execute this
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.
Right - my point is that prepareClusterConnection always returns userCreated: true (or at least it's not obvious in which case it will return false for userCreated. In the already connected state, we wouldn't invoke this method.
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.
ah, got it! yes, good point since we error if something goes wrong. I can remove that from now, I think we'll need to do more work in the connectCluster anyways but this is out of scope here, so let me simplify
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.
Too lazy to test it, but during manual testing, did you experience the model correctly relaying the url to users?
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 couldn't find the LLM propagating the link. But it does show up in the tool call logs if the toggle is clicked
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.
Can we try tweaking the response and seeing what the output is? E.g. we could add something like: Note to LLM Agent: it is important to include the following link in your response to the user in case they want to get more information about the temporary user created
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.
that did the trick!
Screenshot 2025年09月03日 at 13 42 54
* main: feat: add more details about atlas connect flow - MCP-124 (#500) chore: extend library interfaces to allow injecting a custom connection error handler MCP-132 (#502) fix: start mcp even if connection fails - [MCP-140] (#503) fix: allow connect tool on readOnly mode (#499) chore: warn about the usage of deprecated cli arguments MCP-107 (#493) ci: add ipAccessList after creating project (#496)
Uh oh!
There was an error while loading. Please reload this page.
Proposed changes
Checklist