refactor(toolsets): renamed Profile to Toolset#309
Conversation
manusa
force-pushed
the
refactor/profiles
branch
from
2da486c to
935d05c
Compare
As a prior step to providing support for toolsets this change repurposes the current work in profiles which partially aligns with the toolsets expected features Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
force-pushed
the
refactor/profiles
branch
from
935d05c to
5c42009
Compare
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
This looks good to me. None of my comments are blockers.
| cmd.Flags().StringVar(&o.Port, "port", o.Port, "Start a streamable HTTP and SSE HTTP server on the specified port (e.g. 8080)") | ||
| cmd.Flags().StringVar(&o.SSEBaseUrl, "sse-base-url", o.SSEBaseUrl, "SSE public base URL to use when sending the endpoint message (e.g. https://example.com)") | ||
| cmd.Flags().StringVar(&o.Kubeconfig, "kubeconfig", o.Kubeconfig, "Path to the kubeconfig file to use for authentication") | ||
| cmd.Flags().StringVar(&o.Profile, "profile", o.Profile, "MCP profile to use (one of: "+strings.Join(mcp.ProfileNames, ", ")+")") |
There was a problem hiding this comment.
If kubernetes-mcp-server was GA'ed, we need to first deprecate this flag to not cause any breakages. However, I think we are fine to update the flag name now.
There was a problem hiding this comment.
I didn't deprecate it because as of now, the flag was useless.
It wasn't strictly no-op, but since there was just a profile, it had no effect.
There was a problem hiding this comment.
That is fine. But from the user's perspective kubernetes-mcp-server --profile=invalid was working but now it starts failing.
There was a problem hiding this comment.
that should not be working in v0.0.49.
Running the command should print the validation error and an exit 1.
There was a problem hiding this comment.
$ npx -y kubernetes-mcp-server@latest --profile=invalid
Error: invalid profile name: invalid, valid names are: full
Usage:
...|
|
||
| cmd.Flags().BoolVar(&o.Version, "version", o.Version, "Print version information and quit") | ||
| cmd.Flags().IntVar(&o.LogLevel, "log-level", o.LogLevel, "Set the log level (from 0 to 9)") | ||
| cmd.Flags().StringVar(&o.ConfigPath, "config", o.ConfigPath, "Path of the config file. Each profile has its set of defaults.") |
There was a problem hiding this comment.
The removal of this statement is to fix the ambiguity or due to a behavior change in profiles/toolsets?
There was a problem hiding this comment.
When I envisioned profiles, it was not only about tool and prompt groups but also about default settings for other MCP behaviors.
I never implemented that, and I don't think providing defaults is part of what we want from the toolsets feature.
I'm just removing it because it's setting inaccurate expectations and is also unrelated to config.
| type Toolset interface { | ||
| GetName() string | ||
| GetDescription() string | ||
| GetTools(s *Server) []server.ServerTool |
There was a problem hiding this comment.
I'm not sure how can we achieve go-sdk transition by using server.ServerTool but I trust your judgments. (This is independent from this PR, as there is no actual change)
There was a problem hiding this comment.
You'll see in the follow up PRs.
The idea is to eventually make the toolset definition agnostic of the underlying MCP implementation.
This will actually be much easier with go-sdk, but I still want to do it with the m3labs implementation to be able to have a working snapshot in case there's something wrong with go-sdk and we need to revert.
2 participants
As a prior step to providing support for toolsets
this change repurposes the current work in profiles which partially aligns with the toolsets expected features
/cc @mrunalp @ardaguclu