-
Notifications
You must be signed in to change notification settings - Fork 700
chore: groundwork for multi-user to server #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR lays the groundwork for multi‐user support by updating all GitHub client calls to accept a function returning a per-request GitHub client instead of a global client. Key changes include modifying tool/resource handler function signatures, updating tests to wrap the client in a closure, and propagating these changes across resources, pull requests, issues, and code scanning tools.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
File | Description |
---|---|
pkg/github/server_test.go | Updated GetMe call to use a client-returning lambda. |
pkg/github/server.go | Introduced a getClient function to extract authToken from context and used it for all GitHub client invocations. |
pkg/github/search*.go, repositories*.go, pullrequests*.go, issues*.go, code_scanning*.go | Updated function signatures and calls to pass a getClient closure, ensuring per-request client configuration. |
Comments suppressed due to low confidence (1)
pkg/github/server.go:21
- [nitpick] Consider extracting the string literal "authToken" into a named constant to improve clarity and maintainability in contexts where it's used.
authToken, ok := ctx.Value("authToken").(string)
dbf1375
to
b4ca6a9
Compare
pkg/github/code_scanning.go
Outdated
@@ -13,7 +13,7 @@ import ( | |||
"github.com/mark3labs/mcp-go/server" | |||
) | |||
|
|||
func GetCodeScanningAlert(client *github.Client, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { | |||
func GetCodeScanningAlert(getClient func(ctx context.Context) (*github.Client, error), t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we create a type with this function signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I knew I should really - not hard to fix.
b4ca6a9
to
f210d9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved but minor request. Happy for you to bypass under the assumption you just address this if you make any changes!
pkg/github/server_test.go
Outdated
@@ -18,7 +18,7 @@ import ( | |||
func Test_GetMe(t *testing.T) { | |||
// Verify tool definition | |||
mockClient := github.NewClient(nil) | |||
tool, _ := GetMe(mockClient, translations.NullTranslationHelper) | |||
tool, _ := GetMe(func(_ context.Context) (*github.Client, error) { return mockClient, nil }, translations.NullTranslationHelper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wouldn't mind I think a lot of noise could go away with:
func stubGetClientFn(client *github.Client) GetClientFn {
return func(_ context.Context) (*github.Client, error) {
return client, nil
}
}
func Test_GetMe(t *testing.T) {
// Verify tool definition
mockClient := github.NewClient(nil)
tool, _ := GetMe(stubGetClientFn(mockClient), translations.NullTranslationHelper)
...
Or perhaps:
func stubbedGetClientFn() GetClientFn {
return func(_ context.Context) (*github.Client, error) {
return github.NewClient(nil), nil
}
}
func Test_GetMe(t *testing.T) {
// Verify tool definition
tool, _ := GetMe(stubbedGetClientFn(), translations.NullTranslationHelper)
...
Slight preference for the former because it's clearer how the client is being injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made minor edit to names above, just in case you'd already read it.
f210d9a
to
88b709d
Compare
In order to facilitate multi-user (MCP Host) to server communication, we need to support new
*github.Client
clients per request, rather than globally.This change is a NOOP that makes it possible.