Make tiger_service_create MCP tool set default service ID#41
Make tiger_service_create MCP tool set default service ID#41nathanjcochran merged 2 commits intomainfrom
tiger_service_create MCP tool set default service ID#41Conversation
| cfg, err := config.Load() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load config: %w", err) | ||
| } |
There was a problem hiding this comment.
Idk why we were re-loading the config here. I don't think it's necessary to reload it, so I updated the code to just pass in the config that was already loaded.
There was a problem hiding this comment.
It doesn't "re-load" so much as grab a copy. Agreed that this is weird/unnecessary, and passing in is better. There are still things like this I'd like to improve about the config, but I was avoiding broader changes in #36
| // loadProjectID loads fresh config and returns the current project ID | ||
| func (s *Server) loadProjectID() (string, error) { | ||
| // loadConfigWithProjectID loads fresh config and validates that project ID is set | ||
| func (s *Server) loadConfigWithProjectID() (*config.Config, error) { | ||
| // Load fresh config | ||
| cfg, err := config.Load() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to load config: %w", err) | ||
| return nil, fmt.Errorf("failed to load config: %w", err) | ||
| } | ||
|
|
||
| if cfg.ProjectID == "" { | ||
| return "", fmt.Errorf("project ID is required. Please run 'tiger auth login' with --project-id") | ||
| return nil, fmt.Errorf("project ID is required. Please run 'tiger auth login' with --project-id") | ||
| } | ||
| return cfg.ProjectID, nil | ||
| return cfg, nil |
There was a problem hiding this comment.
Updated this to return the full config instead of just the project ID (while still validating that the project ID is set).
There was a problem hiding this comment.
This function is weird, and probably unnecessary. Why is it attached to Server, when s isn't even used? More config weirdness to clean up later, I'm not worried about it now.
There was a problem hiding this comment.
This function is called at the start of every MCP tool call, and it does encapsulate the common projectID validation and error messages so they aren't repeated every time, which seems worthwhile to me. If anything, there's probably more shared logic that we could encapsulate in this function (though the name and return type would probably need to change).
Why is it attached to Server, when s isn't even used?
You're right that it currently doesn't need to have a receiver, but it also doesn't really bother me, fwiw. Lots of people do this as a way of organizing/namespacing functions within a package. I think long-term, it probably makes sense to be related to the Server type. For instance, if we weren't using the global viper instance (and I'd personally prefer if we weren't, as even viper itself discourages its use), we'd probably need to access the viper instance (or something encapsulating it) via the s *Server receiver in order to load the config.
| // Set as default service if requested (defaults to true) | ||
| if input.SetDefault { | ||
| if err := cfg.Set("service_id", serviceID); err != nil { | ||
| // Log warning but don't fail the service creation | ||
| logging.Debug("MCP: Failed to set service as default", zap.Error(err)) | ||
| } else { | ||
| logging.Debug("MCP: Set service as default", zap.String("service_id", serviceID)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the main change.
| cfg, err := config.Load() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load config: %w", err) | ||
| } |
There was a problem hiding this comment.
It doesn't "re-load" so much as grab a copy. Agreed that this is weird/unnecessary, and passing in is better. There are still things like this I'd like to improve about the config, but I was avoiding broader changes in #36
| // loadProjectID loads fresh config and returns the current project ID | ||
| func (s *Server) loadProjectID() (string, error) { | ||
| // loadConfigWithProjectID loads fresh config and validates that project ID is set | ||
| func (s *Server) loadConfigWithProjectID() (*config.Config, error) { | ||
| // Load fresh config | ||
| cfg, err := config.Load() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to load config: %w", err) | ||
| return nil, fmt.Errorf("failed to load config: %w", err) | ||
| } | ||
|
|
||
| if cfg.ProjectID == "" { | ||
| return "", fmt.Errorf("project ID is required. Please run 'tiger auth login' with --project-id") | ||
| return nil, fmt.Errorf("project ID is required. Please run 'tiger auth login' with --project-id") | ||
| } | ||
| return cfg.ProjectID, nil | ||
| return cfg, nil |
There was a problem hiding this comment.
This function is weird, and probably unnecessary. Why is it attached to Server, when s isn't even used? More config weirdness to clean up later, I'm not worried about it now.
2 participants
Makes the
tiger_service_createMCP tool call set the default service ID if theset_defaultparameter istrue(default:true), similar to how thetiger service createcommand works.I wasn't sure if we wanted this at first, but after playing around, I think we do. It's really nice to be able to easily see service details with
tiger service describeor get the connection string withtiger db connection-stringimmediately after the agent creates a service via the MCP tool.