feat(tool/cloudsqladmin): add cloud-sql-admin-execute-sql-many and cloud-sql-admin-sql-many#3083
feat(tool/cloudsqladmin): add cloud-sql-admin-execute-sql-many and cloud-sql-admin-sql-many#3083duwenxin99 wants to merge 9 commits into
cloud-sql-admin-execute-sql-many and cloud-sql-admin-sql-many#3083Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the postgres-execute-sql-many and postgres-sql-many tools and adds an ExecuteSql method to the Cloud SQL Admin source. Review feedback identifies several improvement opportunities, including the removal of the unused region parameter in both tools and the cleanup of the postgressqlmany configuration by removing the unsupported Parameters field and its associated runtime checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the postgres-execute-sql-many and postgres-sql-many tools, enabling SQL execution on Cloud SQL Postgres instances provided at runtime. The changes encompass tool implementations, documentation, prebuilt configuration updates, and integration tests. Review feedback suggests appending " Tool" to the documentation titles for both tools to maintain consistency.
duwenxin99
force-pushed
the
generic-execute-sql
branch
from
7f7c7bd to
f3d09a4
Compare
There was a problem hiding this comment.
These tools won't be generic postgres so we should move them to a different tool source directory
duwenxin99
force-pushed
the
generic-execute-sql
branch
from
f3d09a4 to
8c86358
Compare
| } | ||
|
|
||
| // GetService returns a new Cloud SQL Admin service for the given access token. | ||
|
|
| project, _ := paramsMap["project"].(string) | ||
| instance, _ := paramsMap["instance"].(string) | ||
| database, _ := paramsMap["database"].(string) |
There was a problem hiding this comment.
We should check to make sure these are valid right?
| func (cfg Config) Initialize(srcs map[string]sources.Source) (tools.Tool, error) { | ||
| infraParams := parameters.Parameters{ | ||
| parameters.NewStringParameter("project", "The GCP project ID."), | ||
| parameters.NewStringParameter("instance", "The Cloud SQL instance ID."), |
There was a problem hiding this comment.
instance id or instance name?
| for _, tc := range tcs { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| api := fmt.Sprintf("http://127.0.0.1:5000/api/tool/%s/invoke", tc.toolName) |
There was a problem hiding this comment.
shouldn't we be testing this on the MCP API now?
|
|
||
| allParams, _, err := parameters.ProcessParameters(cfg.TemplateParameters, cfg.Parameters) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Should we return this error wholesale? Do we need to do some massaging of common errors?
duwenxin99
changed the title
postgres-execute-sql-many and postgres-sql-manyclouds-sql-admin-execute-sql-many and clouds-sql-admin-sql-many
duwenxin99
changed the title
clouds-sql-admin-execute-sql-many and clouds-sql-admin-sql-manycloud-sql-admin-execute-sql-many and cloud-sql-admin-sql-many
4 participants
Add 2 new tools using the Cloud SQL Admin
instances.executeSqlAPI to enable SQL execution against instances provided at runtime: