Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 97 additions & 3 deletions devops-mcp-server/cloudbuild/client/cloudbuildclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ import (
"strings"

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"
)

// contextKey is a private type to use as a key for context values.
Expand All @@ -51,8 +53,18 @@ type CloudBuildClient interface {
GetLatestBuildForTrigger(ctx context.Context, projectID, location, triggerID string) (*cloudbuildpb.Build, error)
ListBuildTriggers(ctx context.Context, projectID, location string) ([]*cloudbuildpb.BuildTrigger, error)
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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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)

}

type BuildInfo struct {
BuildDetails *cloudbuildpb.Build
Logs string
}



Comment on lines +66 to +67

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These extra blank lines can be removed to improve code conciseness.

// NewCloudBuildClient creates a new Cloud Build client.
func NewCloudBuildClient(ctx context.Context) (CloudBuildClient, error) {
c, err := cloudbuild.NewClient(ctx)
Expand All @@ -65,13 +77,23 @@ func NewCloudBuildClient(ctx context.Context) (CloudBuildClient, error) {
return nil, fmt.Errorf("failed to create Cloud Build service: %v", err)
}

return &CloudBuildClientImpl{v1client: c, legacyClient: c2}, nil
loggingClient, err := logging.NewClient(ctx)
if err != nil {
return nil, fmt.Errorf("failed to create Logging client: %v", err)
}

return &CloudBuildClientImpl{
v1client: c,
legacyClient: c2,
loggingClient: loggingClient,
}, nil
Comment on lines +85 to +89

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
return &CloudBuildClientImpl{
v1client: c,
legacyClient: c2,
loggingClient: loggingClient,
}, nil
return &CloudBuildClientImpl{
v1client: c,
legacyClient: c2,
loggingClient: loggingClient,
}, nil

}

// CloudBuildClientImpl is an implementation of the CloudBuildClient interface.
type CloudBuildClientImpl struct {
v1client *cloudbuild.Client
legacyClient *build.Service
loggingClient *logging.Client
}

// CreateCloudBuildTrigger creates a new build trigger.
Expand Down Expand Up @@ -181,3 +203,75 @@ func (c *CloudBuildClientImpl) RunBuildTrigger(ctx context.Context, projectID, l
}
return op, nil
}


func (c *CloudBuildClientImpl) ListBuilds(ctx context.Context, projectID, location string) ([]*cloudbuildpb.Build, error) {
req := &cloudbuildpb.ListBuildsRequest{
Parent: fmt.Sprintf("projects/%s/locations/%s", projectID, location),
}
it := c.v1client.ListBuilds(ctx, req)
var builds []*cloudbuildpb.Build
for {
build, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
return nil, fmt.Errorf("failed to list builds: %v", err)
}
builds = append(builds, build)
}
return builds, nil
}

func (c *CloudBuildClientImpl) GetBuildInfo(ctx context.Context, projectID, location, buildID string) (BuildInfo, error) {
req := &cloudbuildpb.GetBuildRequest{
Name: fmt.Sprintf("projects/%s/locations/%s/builds/%s", projectID, location, buildID),
}
build, err := c.v1client.GetBuild(ctx, req)
if err != nil {
return BuildInfo{}, fmt.Errorf("failed to get build info: %w", err)
}
info := BuildInfo{BuildDetails: build}
logReq := &loggingpb.ListLogEntriesRequest{
ResourceNames: []string{fmt.Sprintf("projects/%s", projectID)},
Filter: fmt.Sprintf(`resource.type="build" AND resource.labels.build_id="%s" AND logName="projects/%s/logs/cloudbuild"`, buildID, projectID),
}
it := c.loggingClient.ListLogEntries(ctx, logReq)
var logs []string
for {
entry, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
return BuildInfo{}, fmt.Errorf("failed to list log entries: %w", err)
}
var logMessage string
switch payload := entry.Payload.(type) {
case *loggingpb.LogEntry_TextPayload:
logMessage = payload.TextPayload
case *loggingpb.LogEntry_JsonPayload:
logMessage = fmt.Sprintf("%v", payload.JsonPayload)
Comment on lines +254 to +255

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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)
				}

case *loggingpb.LogEntry_ProtoPayload:
logMessage = fmt.Sprintf("%v", payload.ProtoPayload)
default:
return BuildInfo{}, fmt.Errorf("unknown log entry payload type")
}
logs = append(logs, logMessage)
}
info.Logs = strings.Join(logs, "\n")
return info, nil
}

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},
}
Comment on lines +267 to +271

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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},
	}

ops, err := c.v1client.CreateBuild(ctx, req)
if err != nil {
return nil, fmt.Errorf("failed to start build: %v", err)
}
return ops, nil
}
64 changes: 64 additions & 0 deletions devops-mcp-server/cloudbuild/cloudbuild.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
cloudbuildclient "devops-mcp-server/cloudbuild/client"
iamclient "devops-mcp-server/iam/client"
resourcemanagerclient "devops-mcp-server/resourcemanager/client"

cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb"
)

// Handler holds the clients for the cloudbuild service.
Expand All @@ -39,6 +41,9 @@ func (h *Handler) Register(server *mcp.Server) {
addCreateTriggerTool(server, h.CbClient, h.IClient, h.RClient)
addRunTriggerTool(server, h.CbClient)
addListTriggersTool(server, h.CbClient)
addListBuildsTool(server, h.CbClient)
addGetBuildInfoTool(server, h.CbClient)
addStartBuildTool(server, h.CbClient)
}

type RunTriggerArgs struct {
Expand Down Expand Up @@ -146,3 +151,62 @@ func IsValidServiceAccount(sa string) bool {
var saRegex = regexp.MustCompile(`^serviceAccount:[a-z0-9-]+@[a-z0-9-]+\.iam\.gserviceaccount\.com$`)
return saRegex.MatchString(sa)
}

type ListBuildsArgs struct {
ProjectID string `json:"project_id" jsonschema:"The Google Cloud project ID."`
Location string `json:"location" jsonschema:"The Google Cloud location for the builds."`
}

type GetBuildInfoArgs struct {
ProjectID string `json:"project_id" jsonschema:"The Google Cloud project ID."`
Location string `json:"location" jsonschema:"The Google Cloud location for the build."`
BuildID string `json:"build_id" jsonschema:"The ID of the build."`
}

type StartBuildArgs struct {
ProjectID string `json:"project_id" jsonschema:"The Google Cloud project ID."`
Location string `json:"location" jsonschema:"The Google Cloud location for the build."`
Bucket string `json:"bucket" jsonschema:"The Cloud Storage bucket where the source is located."`
Object string `json:"object" jsonschema:"The Cloud Storage object (file) where the source is located."`
}

func addListBuildsTool(server *mcp.Server, cbClient cloudbuildclient.CloudBuildClient) {
listBuildsToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, args ListBuildsArgs) (*mcp.CallToolResult, any, error) {
res, err := cbClient.ListBuilds(ctx, args.ProjectID, args.Location)
if err != nil {
return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to list builds: %w", err)
}
return &mcp.CallToolResult{}, map[string]any{"builds": res}, nil
}
mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.list_builds", Description: "Lists all Cloud Builds in a given location and project."}, listBuildsToolFunc)
}

func addGetBuildInfoTool(server *mcp.Server, cbClient cloudbuildclient.CloudBuildClient) {
getBuildInfoToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, args GetBuildInfoArgs) (*mcp.CallToolResult, any, error) {
res, err := cbClient.GetBuildInfo(ctx, args.ProjectID, args.Location, args.BuildID)
if err != nil {
return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to get build info: %w", err)
}
return &mcp.CallToolResult{}, res, nil
}
mcp.AddTool(server, &mcp.Tool{Name: "cloudbuild.get_build_info", Description: "Gets information about a specific Cloud Build."}, getBuildInfoToolFunc)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

}

func addStartBuildTool(server *mcp.Server, cbClient cloudbuildclient.CloudBuildClient) {
startBuildToolFunc := func(ctx context.Context, req *mcp.CallToolRequest, args StartBuildArgs) (*mcp.CallToolResult, any, error) {
source:= &cloudbuildpb.Source{
Source: &cloudbuildpb.Source_StorageSource{
StorageSource: &cloudbuildpb.StorageSource{
Bucket: args.Bucket,
Object: args.Object,
},
},
}
res, err := cbClient.StartBuild(ctx, args.ProjectID, source)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
res, err := cbClient.StartBuild(ctx, args.ProjectID, source)
res, err := cbClient.StartBuild(ctx, args.ProjectID, args.Location, source)

if err != nil {
return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to start build: %w", err)
}
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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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)

}
Loading