Use cobra to align with kubernetes #123
Conversation
| type MCPServerOptions struct { | ||
| Version bool | ||
| LogLevel int | ||
| SSEPort int |
There was a problem hiding this comment.
This should be string. For the simplicity of this PR, it is better to change the type in a followup PR.
| Version bool | ||
| LogLevel int | ||
| SSEPort int | ||
| HttpPort int |
| } | ||
|
|
||
| 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)") |
There was a problem hiding this comment.
Maybe instead of having a separate log-level flag, we can directly wire the -v in klog.
|
|
||
| if m.SSEPort > 0 { | ||
| sseServer := mcpServer.ServeSse(m.SSEBaseUrl) | ||
| defer func() { _ = sseServer.Shutdown(ctx) }() |
There was a problem hiding this comment.
We don't pass ctx anywhere apart from here. I assume that we should pass the context to internal tools.
| } | ||
| } | ||
|
|
||
| if err := mcpServer.ServeStdio(); err != nil && !errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
Since we don't pass context in anywhere, it is not possible to get context.Cancelled. We need to wire this.
|
@manusa would you please have a look at this, when you have a chance?. Thank you. |
|
Checking the failures unit test failures... |
probably related to the tightly coupling of the cmd outputs and cobra internals. |
| } | ||
|
|
||
| func TestListOutput(t *testing.T) { | ||
| t.Run("available", func(t *testing.T) { |
There was a problem hiding this comment.
I have to remove this test. Because in Cobra, there is no easy way to test kubernetes-mcp-server --help. Maybe we don't need to test this anyway?
There was a problem hiding this comment.
I have what I call test-compulsive disorder (TCD) 😅
Disable/skip the tests that are giving you problems and add a TODO comment, I'll try to fix them at another time
There was a problem hiding this comment.
I've successfully added back this test by using captureOutput.
|
@manusa this is ready for your review, when you have a chance. |
| func (m *MCPServerOptions) Complete() error { | ||
| m.initializeLogging() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
What's the purpose of this method?
Is it to match this plugin structure?
There was a problem hiding this comment.
It is used to initialization steps like kubeconfig, dynamic client, etc. Conceptually, when Run is called, everything should be ready to work.
There was a problem hiding this comment.
Complete -> all the initializations are completed
Validate -> everything is validated
Run -> should run the actual command
There was a problem hiding this comment.
That said; these https://github.com/manusa/kubernetes-mcp-server/blob/2c18ca0822b51bf7de8d41baada87475cd446525/pkg/kubernetes-mcp-server/cmd/root.go#L126-L138 can be moved to Complete and Validate
This https://github.com/manusa/kubernetes-mcp-server/blob/2c18ca0822b51bf7de8d41baada87475cd446525/pkg/kubernetes-mcp-server/cmd/root.go#L140-L143 should be a separate command kubernete-mcp-server version instead of --version
and There should be 3 managers -- stdio, sse, the other and Run should execute the requested onemcp.NewServer is nicely wired already.
2 participants
https://github.com/kubernetes/sample-cli-plugin provides a skeleton of how kubernetes tools can be. This PR makes necessary changes to align with sample-cli-plugin. Thanks to that, we'll remove some dependencies from go.mod.
This PR must be no-op.