chore: Improve access list and connect experienece [MCP-5] #372
Conversation
Pull Request Test Coverage Report for Build 16350134895Warning: 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 |
src/common/atlas/accessListUtils.ts
Outdated
| */ | ||
| export async function ensureCurrentIpInAccessList(apiClient: ApiClient, projectId: string): Promise<void> { | ||
| // Get the current public IP | ||
| const entry = await makeCurrentIpAccessListEntry(apiClient, projectId, "Added by MCP pre-run access list helper"); |
There was a problem hiding this comment.
[nit] Is that comment helpful externally? If I was a user and saw that, I wouldn't know what it means.
There was a problem hiding this comment.
I updated a bit more, but I think it's helpful to know where the new access list entry came from
| @@ -2,6 +2,7 @@ import { z } from "zod"; | |||
| import { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; | |||
| import { AtlasToolBase } from "../atlasTool.js"; | |||
| import { ToolArgs, OperationType } from "../../tool.js"; | |||
| import { makeCurrentIpAccessListEntry } from "../../../common/atlas/accessListUtils.js"; | |||
|
|
|||
| const DEFAULT_COMMENT = "Added by Atlas MCP"; | |||
There was a problem hiding this comment.
This is duplicated now - should we move it to accessListUtils?
src/common/atlas/accessListUtils.ts
Outdated
| // 409 Conflict: entry already exists, ignore | ||
| return; | ||
| } | ||
| logger.debug( |
There was a problem hiding this comment.
This is silently ignoring the error - does it make sense that we do that? I assume that this could give users the false impression everything is working and then hit an issue with the IP address config at a later point?
There was a problem hiding this comment.
Yes for now, there are different configurations where user does not have access to add access list but has accsess to proceed with the tool call, I'd like to avoid causing errors and having the logs to troubleshoot for now
There was a problem hiding this comment.
To be clear - I'm not sure about those scenarios but was trying to avoid causing disruption. If you think the risk is too low I can throw the error back. Thanks!
There was a problem hiding this comment.
Yeah, not sure either, just not ideal that this function seems like a prerequisite for many features, but we're not communicating to the user that something might have gone wrong. I would say this PR is a clear improvement to the status quo, and the worst case of this failing and us logging it is no worse than what we currently have, so fine with keeping it as is. I guess we can at least bump the log level to warn so that it's a bit more prominent.
src/common/atlas/accessListUtils.ts
Outdated
| // 409 Conflict: entry already exists, ignore | ||
| return; | ||
| } | ||
| logger.debug( |
There was a problem hiding this comment.
Yeah, not sure either, just not ideal that this function seems like a prerequisite for many features, but we're not communicating to the user that something might have gone wrong. I would say this PR is a clear improvement to the status quo, and the worst case of this failing and us logging it is no worse than what we currently have, so fine with keeping it as is. I guess we can at least bump the log level to warn so that it's a bit more prominent.
Proposed changes
Checklist