chore: move parameters to internal/util#1907
Conversation
bf0569f to
018963f
Compare
609af8e to
e733977
Compare
To facilitate the transition of moving invocation implementation to Source, we will have to move parameter to `internal/util`. This approach is crucial because certain parameters may not be fully resolvable pre-implementation. Since both `internal/sources` and `internal/tools` will need access to `parameters`, it will be more relevant to move parameters implementation to utils.
|
/gemini help review this PR |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to move the parameters implementation to internal/util, making it accessible to both internal/sources and internal/tools. The changes are extensive and mostly correct. However, I've identified two critical copy-paste errors that introduce duplicate parameters in alloydbcreateuser and alloydbgetinstance tools, which would likely cause runtime failures. My review includes suggestions to remove the redundant code.
| tools.NewStringParameterWithRequired("password", "A secure password for the new user. Required only for ALLOYDB_BUILT_IN userType.", false), | ||
| tools.NewArrayParameterWithDefault("databaseRoles", []any{}, "Optional. A list of database roles to grant to the new user (e.g., ['pg_read_all_data']).", tools.NewStringParameter("role", "A single database role to grant to the user (e.g., 'pg_read_all_data').")), | ||
| tools.NewStringParameter("userType", "The type of user to create. Valid values are: ALLOYDB_BUILT_IN and ALLOYDB_IAM_USER. ALLOYDB_IAM_USER is recommended."), | ||
| parameters.NewStringParameter("project", "The GCP project ID."), |
There was a problem hiding this comment.
There is a duplicate project parameter being added to allParameters. The projectParam variable is already being added on line 83. This line is redundant and will cause issues with parameter parsing, such as panics due to duplicate keys in maps.
| parameters.NewStringParameter("location", "The location of the instance (e.g., 'us-central1')."), | ||
| parameters.NewStringParameter("cluster", "The ID of the cluster."), | ||
| parameters.NewStringParameter("instance", "The ID of the instance."), |
There was a problem hiding this comment.
The parameters location, cluster, and instance are duplicated. This appears to be a copy-paste error and will cause issues with parameter parsing, such as panics due to duplicate keys in maps.
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to move the parameters implementation to internal/util to facilitate its use across different packages. The changes are extensive, touching many files to update import paths and type references. Overall, the refactoring is well-executed. However, I've identified a couple of critical copy-paste errors that introduce duplicate parameters, which would lead to runtime panics. These need to be addressed to ensure the stability of the application.
| tools.NewStringParameterWithRequired("password", "A secure password for the new user. Required only for ALLOYDB_BUILT_IN userType.", false), | ||
| tools.NewArrayParameterWithDefault("databaseRoles", []any{}, "Optional. A list of database roles to grant to the new user (e.g., ['pg_read_all_data']).", tools.NewStringParameter("role", "A single database role to grant to the user (e.g., 'pg_read_all_data').")), | ||
| tools.NewStringParameter("userType", "The type of user to create. Valid values are: ALLOYDB_BUILT_IN and ALLOYDB_IAM_USER. ALLOYDB_IAM_USER is recommended."), | ||
| parameters.NewStringParameter("project", "The GCP project ID."), |
There was a problem hiding this comment.
This line duplicates the project parameter. projectParam is already added to allParameters on the preceding line. This duplication will cause a runtime error when initializing the tool. Please remove this line.
| parameters.NewStringParameter("location", "The location of the instance (e.g., 'us-central1')."), | ||
| parameters.NewStringParameter("cluster", "The ID of the cluster."), | ||
| parameters.NewStringParameter("instance", "The ID of the instance."), |
There was a problem hiding this comment.
These lines are duplicates of the location, cluster, and instance parameters defined on lines 84-86. This duplication will cause a runtime error. Please remove these duplicated lines.
To facilitate the transition of moving invocation implementation to Source, we will have to move parameter to
internal/util. This approach is crucial because certain parameters may not be fully resolvable pre-implementation. Since bothinternal/sourcesandinternal/toolswill need access toparameters, it will be more relevant to move parameters implementation to utils.