Sdk migration - refactor tools and client into packages and add list_terraform_orgs tool - #493
Sdk migration - refactor tools and client into packages and add list_terraform_orgs tool#493MulyaP wants to merge 5 commits into
Conversation
…e terraform prefix
| trueVal := true | ||
| falseVal := false |
There was a problem hiding this comment.
This is gross and no doubt we're gonna need to do it in a bunch of places. Instead, we could make a helper func for this like:
func ptr[T any](v T) *T {
return &v
}There was a problem hiding this comment.
Agree. Made a utils folder and utils.go file and added the helper function there.
| "github.com/hashicorp/terraform-mcp-server/pkg/mcp-official/client" | ||
| ) | ||
|
|
||
| type OrganizationSummary struct { |
There was a problem hiding this comment.
With the new code let's be consistent and put the godoc comments above every struct definition
| PageSize int `json:"pageSize,omitempty" jsonschema:"Results per page for pagination (min 1, max 100)"` | ||
| } | ||
|
|
||
| func ListTerraformOrganizationsTool() *mcp.Tool { |
There was a problem hiding this comment.
Something that's missing here is injecting the logger dependency.
It gets passed in at the RegisterTools() call here: https://github.com/hashicorp/terraform-mcp-server/blob/main/pkg/mcp-official/tools.go#L9
But then it just gets swallowed, and the code here defaults to using log from the import.
There was a problem hiding this comment.
I looked into this and simply passing logger into the handler func argument in mcp.AddTool will not work because of how of its designed. One solution that works here is to make the handler a closure function which looks something like this:
func ListTerraformOrganizationsFunc(logger *log.Logger) func(ctx context.Context, req *mcp.CallToolRequest, input ListOrganizationsArguments) (*mcp.CallToolResult, *OrganizationSummaryList, error) {
return func(ctx context.Context, req *mcp.CallToolRequest, input ListOrganizationsArguments) (*mcp.CallToolResult, *OrganizationSummaryList, error) {
log.Info("ListTerraformOrganizations for official mcp go-sdk called.. dkfhgbdf")
}
}
In my opinion, this makes it unnecessarily convoluted and importing log seems like a good alternative. I will try to find other methods with which we can achieve this in a simpler way.
|
|
||
| func ListWorkspacesFunc(ctx context.Context, request *mcp.CallToolRequest, input ListWorkspacesArguments) (*mcp.CallToolResult, *WorkspaceSummaryList, error) { | ||
| log.Info("ListWorkspaces for official mcp go-dk called..") | ||
| log.Info("ListWorkspaces for official mcp go-sdk called..") |
There was a problem hiding this comment.
This log message isn't very useful, there is already a tool call logger middleware that's going to get ported across: https://github.com/Marin2409/terraform-mcp-server/blob/main/pkg/client/tool_logging_middleware.go
There was a problem hiding this comment.
I agree, I should remove it. It was just there for me to know which tool endpoint was called (official or mark3labs) when testing it.
4 participants
This PR adds
list_terraform_orgtool using the official go-sdk for MCP and refactors the official sdk code into packages like in mark2labs/mcp-go code.PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.