Conversation
|
/gemini help |
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 |
|
/gemini summary |
![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 adds new MCP tools for interacting with Google Cloud Build, including listing builds, getting build information, and starting new builds. The implementation is mostly solid, but there are a few issues to address. The most critical is a bug in the start_build tool where the specified location is ignored, and the build is always created in the global location. Additionally, there are some inconsistencies in tool naming between the code and the PR description, and a misleading tool description. I've also included some suggestions for improving code style and log message formatting.
| RunBuildTrigger(ctx context.Context, projectID, location, triggerID, branch, tag, commitSha string) (*cloudbuild.RunBuildTriggerOperation, error) | ||
| ListBuilds(ctx context.Context, projectID, location string) ([]*cloudbuildpb.Build, error) | ||
| GetBuildInfo(ctx context.Context, projectID, location, buildID string) (BuildInfo, error) | ||
| StartBuild(ctx context.Context, projectID string, source *cloudbuildpb.Source) (*cloudbuild.CreateBuildOperation, error) |
There was a problem hiding this comment.
The StartBuild function should accept a location parameter. Currently, the implementation hardcodes it to global, but the corresponding tool cloudbuild.start defines a location argument, which is then ignored. This is a bug that needs to be fixed across the interface and implementation.
| StartBuild(ctx context.Context, projectID string, source *cloudbuildpb.Source) (*cloudbuild.CreateBuildOperation, error) | |
| StartBuild(ctx context.Context, projectID, location string, source *cloudbuildpb.Source) (*cloudbuild.CreateBuildOperation, error) |
| func (c *CloudBuildClientImpl) StartBuild(ctx context.Context, projectID string, source *cloudbuildpb.Source) (*cloudbuild.CreateBuildOperation, error) { | ||
| req := &cloudbuildpb.CreateBuildRequest{ | ||
| Parent: fmt.Sprintf("projects/%s/locations/global", projectID), | ||
| Build: &cloudbuildpb.Build{Source: source}, | ||
| } |
There was a problem hiding this comment.
This implementation of StartBuild should take location as an argument and use it to construct the Parent field for the request, instead of hardcoding global. This fixes a bug where the user-provided location is ignored.
func (c *CloudBuildClientImpl) StartBuild(ctx context.Context, projectID, location string, source *cloudbuildpb.Source) (*cloudbuild.CreateBuildOperation, error) {
req := &cloudbuildpb.CreateBuildRequest{
Parent: fmt.Sprintf("projects/%s/locations/%s", projectID, location),
Build: &cloudbuildpb.Build{Source: source},
}| }, | ||
| }, | ||
| } | ||
| res, err := cbClient.StartBuild(ctx, args.ProjectID, source) |
There was a problem hiding this comment.
The location argument from StartBuildArgs is being ignored here. You should pass it to the cbClient.StartBuild function. This change depends on updating the StartBuild method signature in the client to accept a location.
| res, err := cbClient.StartBuild(ctx, args.ProjectID, source) | |
| res, err := cbClient.StartBuild(ctx, args.ProjectID, args.Location, source) |
| } | ||
| return &mcp.CallToolResult{}, res, nil | ||
| } | ||
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.start_build", Description: "Starts a new Cloud Build with the given source locally."}, startBuildToolFunc) |
There was a problem hiding this comment.
The tool name cloudbuild.start_build is inconsistent with the PR description (cloudbuild.start). Additionally, the description Starts a new Cloud Build with the given source locally. is misleading, as the source is from Google Cloud Storage, not a local directory. Please update both for clarity and consistency.
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.start_build", Description: "Starts a new Cloud Build with the given source locally."}, startBuildToolFunc) | |
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.start", Description: "Starts a new Cloud Build from a source in Google Cloud Storage."}, startBuildToolFunc) |
| cloudbuild "cloud.google.com/go/cloudbuild/apiv1/v2" | ||
| cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb" | ||
|
|
||
| logging "cloud.google.com/go/logging/apiv2" | ||
| build "google.golang.org/api/cloudbuild/v1" | ||
| "google.golang.org/api/iterator" | ||
| "google.golang.org/protobuf/types/known/timestamppb" | ||
|
|
||
| cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb" | ||
| loggingpb "cloud.google.com/go/logging/apiv2/loggingpb" | ||
| ) |
There was a problem hiding this comment.
The import block is not organized according to Go conventions. Running goimports would automatically group standard library, third-party, and internal packages, improving readability and maintainability.
|
|
||
|
|
There was a problem hiding this comment.
These extra blank lines can be removed to improve code conciseness.
| return &CloudBuildClientImpl{ | ||
| v1client: c, | ||
| legacyClient: c2, | ||
| loggingClient: loggingClient, | ||
| }, nil |
There was a problem hiding this comment.
The formatting of this struct literal is inconsistent. Running gofmt or goimports would fix the alignment and spacing, making the code more readable and consistent with Go standards.
| return &CloudBuildClientImpl{ | |
| v1client: c, | |
| legacyClient: c2, | |
| loggingClient: loggingClient, | |
| }, nil | |
| return &CloudBuildClientImpl{ | |
| v1client: c, | |
| legacyClient: c2, | |
| loggingClient: loggingClient, | |
| }, nil |
| case *loggingpb.LogEntry_JsonPayload: | ||
| logMessage = fmt.Sprintf("%v", payload.JsonPayload) |
There was a problem hiding this comment.
Using fmt.Sprintf("%v", ...) for a JSON payload will produce the Go struct's string representation, not a valid JSON string. This can be hard to parse or read. Consider marshaling the payload to a JSON string using protojson.Marshal. You will need to add "google.golang.org/protobuf/encoding/protojson" to your imports.
case *loggingpb.LogEntry_JsonPayload:
jsonBytes, err := protojson.Marshal(payload.JsonPayload)
if err != nil {
logMessage = fmt.Sprintf("failed to marshal json payload to string: %v", err)
} else {
logMessage = string(jsonBytes)
}| } | ||
| return &mcp.CallToolResult{}, res, nil | ||
| } | ||
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.get_build_info", Description: "Gets information about a specific Cloud Build."}, getBuildInfoToolFunc) |
There was a problem hiding this comment.
The tool name cloudbuild.get_build_info is inconsistent with the PR description, which specifies cloudbuild.build_info. Please align the tool name for consistency.
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.get_build_info", Description: "Gets information about a specific Cloud Build."}, getBuildInfoToolFunc) | |
| mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.build_info", Description: "Gets information about a specific Cloud Build."}, getBuildInfoToolFunc) |
Summary of ChangesThis pull request significantly expands the capabilities of the Cloud Build integration by adding three new tools. These tools enable users to list all builds for a given project and region, retrieve detailed information about a specific build including its logs, and initiate new builds directly from Google Cloud Storage. The changes involve updating the Cloud Build client interface and implementation to support these new operations, as well as registering the corresponding tools within the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
1 participant
Added tools under cloud build
cloudbuild.list_builds: List builds for the given project and regioncloudbuild.build_info: This should provide a detailed information about a given build along with build logs.cloudbuild.start: This will allow running a GCB build on a local directory.