feat: func mcp#3155
Conversation
knative-prow
Bot
added
approved
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
4 times, most recently
from
b75b584 to
59f42c0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3155 +/- ##
==========================================
+ Coverage 59.35% 63.87% +4.52%
==========================================
Files 134 150 +16
Lines 13519 13210 -309
==========================================
+ Hits 8024 8438 +414
+ Misses 4552 3783 -769
- Partials 943 989 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
3 times, most recently
from
7330b01 to
be3efaf
Compare
| /pkg/functions/testdata/default_home/go | ||
| /pkg/functions/testdata/default_home/.cache | ||
| /pkg/functions/testdata/migrations/*/.gitignore | ||
| /pkg/creds/auth.json |
|
I'd group I'd than do something like https://github.com/containers/kubernetes-mcp-server/blob/main/pkg/toolsets/core/toolset.go#L23 for registering those. |
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
2 times, most recently
from
961dccc to
ea28447
Compare
twoGiants
left a comment
twoGiants
left a comment
There was a problem hiding this comment.
Hi @lkingland! Great job on the MCP integration 😸 👍
Most of my comments so far are cleanups, refactorings and a few typos. Some are opinions, so you decide if you want to go that route 😸 . Many can probably be done in a follow up.
I finished reviewing up to tools_build_test.go. Not much left. Will submit another after lunch.
The kubernetes-mcp-server toolset registry pattern is good for that use case (dozens of tools across multiple independent domains like Kubernetes, Helm, and Config), but is a bit overly complex here.
For our single-domain, static toolset of only 8 tools, explicit registration in New() is simple, without the indirection of init functions, global registries, etc. Lower cognitive load. I'd say we follow that pattern if our complexity grows 👍 |
twoGiants
left a comment
twoGiants
left a comment
There was a problem hiding this comment.
Left a few more comments.
Looks all good! I have mostly cleanup comments. Can (not must) all be done in a follow PR and would polish the code a bit. But you make the call 😸
knative-prow
Bot
added
the
do-not-merge/work-in-progress
Converting back to draft, so we can make it slightly more... slightly more mature. |
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
7 times, most recently
from
a44f448 to
2594188
Compare
knative-prow
Bot
removed
the
do-not-merge/work-in-progress
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
2 times, most recently
from
0c89862 to
2c6a1bf
Compare
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
from
2c6a1bf to
d77ff89
Compare
| Name = "func" | ||
| Title = "func" | ||
| Version = "0.1.0" |
There was a problem hiding this comment.
Encapsulate non exported.
| Name = "func" | |
| Title = "func" | |
| Version = "0.1.0" | |
| bame = "func" | |
| title = "func" | |
| version = "0.1.0" |
| Name: Name, | ||
| Title: Title, | ||
| Version: Version}, |
There was a problem hiding this comment.
| Name: Name, | |
| Title: Title, | |
| Version: Version}, | |
| Name: name, | |
| Title: title, | |
| Version: version}, |
| func (s *MCPServer) Start() error { | ||
| return server.ServeStdio(s.server) | ||
| } | ||
| var ErrWriteDisabled = errors.New("server is not write enabled") |
There was a problem hiding this comment.
Remove unused.
| var ErrWriteDisabled = errors.New("server is not write enabled") |
| server: mcpServer, | ||
| } | ||
| cmd := exec.CommandContext(ctx, cmdParts[0], cmdParts[1:]...) | ||
| // Commands always execute in current working directory |
There was a problem hiding this comment.
It's the default behavior since cmd.Dir is not set. I would either remove the comment or make it more precise:
| // Commands always execute in current working directory | |
| // cmd.Dir not set - inherits process working directory which is the current working directory |
| } | ||
|
|
||
| // newTestPairWithReadonly returns a ClientSession and Server with the specified readonly mode. | ||
| func newTestPairWithReadonly(t *testing.T, readonly bool) (*mcp.ClientSession, *Server, error) { |
There was a problem hiding this comment.
newTestPairWithReadonly and newTestPair are nearly identical and can be refactored into one function with logic => newTestPair and newTestPairWithReadonly decorating it and setting readonly.
|
|
||
| // resource helpers: | ||
|
|
||
| func newErrorResult(uri string, err error) *mcp.ReadResourceResult { |
There was a problem hiding this comment.
You could put another helper here which would be used in the other resource_*.go files:
func newSuccessResult(uri, mime string, text []byte) *mcp.ReadResourceResult {
return &mcp.ReadResourceResult{
Contents: []*mcp.ResourceContents{
{
URI: uri,
MIMEType: mime,
Text: string(text),
},
},
}
}| } | ||
| } | ||
|
|
||
| func TestResource_Languages(t *testing.T) { |
There was a problem hiding this comment.
This and the next could be one table driven test with two test cases. Would remove the duplication.
There was a problem hiding this comment.
This is pretty good!
The tools_*_test.go are very similar. This file could have a bit more, like common assertions and common setup. But no need to do now. Could be a good-first-issue.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland, twoGiants The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Solid suggestions @twoGiants ; I'll add those changes in an update PR. |
picking knative#3155 Signed-off-by: Matej Vašek <mvasek@redhat.com> Co-authored-by: Luke Kingland <luke@lukekingland.com>
* fix templates ref picking knative#2967 Signed-off-by: Matej Vašek <mvasek@redhat.com> Co-authored-by: David Fridrich <fridrich.david19@gmail.com> * Revert "fix: MCP use "kn func" not "func" cmd (#1380)" This reverts commit 38f804e. Signed-off-by: Matej Vašek <matejvasek@gmail.com> * mcp refactor picking knative#3155 Signed-off-by: Matej Vašek <mvasek@redhat.com> Co-authored-by: Luke Kingland <luke@lukekingland.com> * fixup: compatibility with Go 1.23 The test context was introduced only in Go 1.24 we need however compatibility with Go 1.23. Signed-off-by: Matej Vašek <mvasek@redhat.com> --------- Signed-off-by: Matej Vašek <mvasek@redhat.com> Signed-off-by: Matej Vašek <matejvasek@gmail.com> Co-authored-by: David Fridrich <fridrich.david19@gmail.com> Co-authored-by: Luke Kingland <luke@lukekingland.com>
4 participants
Updates the POC Functions MCP server to be a slightly more mature experimental MCP server.
Enable with FUNC_ENABLE_MCP and start (by configuring one's client LLM) via
func mcp start.