Conversation
dimetron
commented
Nov 18, 2025
dimetron
commented
- Migrated to github.com/modelcontextprotocol/go-sdk
- Added argocd tools
Signed-off-by: Dmytro Rashko <dmitriy.rashko@amdocs.com>
| return &mcp.CallToolResult{ | ||
| Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("istioctl proxy-status failed: %v", err)}}, | ||
| IsError: true, | ||
| }, nil |
There was a problem hiding this comment.
should we return error here too, instead nil?
| namespace := "" | ||
| configType := "all" | ||
|
|
||
| if val, ok := args["pod_name"].(string); ok { |
There was a problem hiding this comment.
nit: I'd extract this functionality into a separate function - something like:
func getValueOrDefault(args map[string]interface{}, field string) string {
if val, ok := args[field].(string); ok {
return val
}
return ""
}
| namespace = val | ||
| } | ||
| if val, ok := args["all_namespaces"].(string); ok { | ||
| allNamespaces = val == "true" |
There was a problem hiding this comment.
should the val be transformed to lower case and then compared?
EItanya
left a comment
EItanya
left a comment
There was a problem hiding this comment.
Leaving the partial review as this PR is quite large. Spoke offline about switching tool function definitions to use generics
There was a problem hiding this comment.
Why do we need this test? I think it should be the responsibility of the upstream library to test this. Maintaining this on our side seems unnecessary
There was a problem hiding this comment.
Same question about testing core mcp library functionality.
There was a problem hiding this comment.
So I understand what you are trying to do here, but I think running the built binary adds an unnecessary level of complexity to the test. Why can't we just use the server object from this repo, and the official mcp-go client?
There was a problem hiding this comment.
I don't think we need 4 separate tests for this, since this is an integration test I think we can just test all in one
- Launch
- init
- List
- Call
| ) | ||
|
|
||
| // StdioTestServer represents a server instance for stdio transport testing | ||
| type StdioTestServer struct { |
There was a problem hiding this comment.
This type seems to be functionally equivalent to the ComprehensiveTestServer, can we re-use that? Also same question about using the mcp-go client instead of recreating all of that logic here again
3 participants
