Conversation
📝 WalkthroughWalkthroughReplaces Run-based CD execution with Cloud Build: introduces CloudBuildStep/CloudBuildArgs and RunCloudBuild/GetBuildStatus, marshals build steps to YAML, injects etag into build env and tags (including Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as BYOC CLI
participant Driver as Local Driver/Runtime
participant GCP as Cloud Build API
participant Logs as Cloud Logging
CLI->>GCP: RunCloudBuild(CloudBuildArgs{stepsYAML, tags:[...,defang-cd], substitutions (DEFANG_ETAG)})
GCP-->>CLI: operationName
loop Poll build
CLI->>Driver: GetBuildStatus(operationName)
Driver->>GCP: Query build by operationName
GCP-->>Driver: Build status (ACTIVATING/RUNNING/COMPLETED/FAILED)
Driver-->>CLI: Status/result (io.EOF on success / error on failure)
end
Note over Logs,CLI: Log-stream CD detection & handling
Logs->>CLI: Stream LogEntry events (build_tags + audit logs)
CLI->>CLI: parse buildTags[]
alt defang-cd build detected
CLI->>CLI: set cdStarted = true
alt build completed
CLI->>CLI: emit ready-services + CD deployment-completed
else build failed
CLI->>CLI: emit deployment-failed
end
else non-CD build
CLI->>CLI: process BUILD_* state transitions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/pkg/clouds/gcp/cloudbuild.go:
- Around line 120-123: The YAML is being unmarshaled directly into
[]*cloudbuildpb.BuildStep using yaml.Unmarshal (args.Steps -> steps), but
protobuf types lack YAML tags so this fails; fix by converting YAML to JSON and
then using protojson.Unmarshal to populate []*cloudbuildpb.BuildStep OR
unmarshal into an intermediate struct (e.g., the CloudBuildStep type used in
byoc.go) with proper `yaml` tags and then map/convert those instances into
cloudbuildpb.BuildStep messages before continuing; update the code around
yaml.Unmarshal, args.Steps, and the variable steps to use protojson.Unmarshal or
the intermediate-to-proto mapping so BuildStep messages are correctly populated.
🧹 Nitpick comments (5)
src/pkg/cli/client/byoc/gcp/byoc_test.go (1)
106-145: Test data (listEntries,tailEntries) is defined but never used.The test cases define
listEntriesandtailEntriesslices, but the mock at lines 142-145 is always initialized with an emptyMockGcpLoggingLister{}. This means the test data has no effect on the test behavior.If this is intentional placeholder data for future tests, consider adding a TODO comment. Otherwise, consider either removing the unused fields or wiring them into the mock.
src/pkg/cli/client/byoc/gcp/stream.go (2)
454-460: Fragile CD start detection based on log message prefix.The detection of CD start via
strings.HasPrefix(msg, " ** Update started")is acknowledged as a HACK. This approach is brittle since any change to the upstream log format (e.g., Pulumi output) could silently break CD log filtering.Consider documenting the expected message format or adding a more robust marker (e.g., a structured log field or a specific tag) if feasible in future iterations.
699-701: Potential nil pointer dereference onauditLog.GetStatus().At line 700,
auditLog.GetStatus().GetMessage()is called without a nil check. WhileGetMessage()is nil-safe, ifGetStatus()returns nil, the error message would be empty, which may not provide useful context. Consider including the severity or a fallback message.🔎 Suggested improvement
if entry.Severity == logtype.LogSeverity_ERROR { - return nil, client.ErrDeploymentFailed{Message: auditLog.GetStatus().GetMessage()} + msg := "Cloud Build failed" + if auditLog.GetStatus() != nil { + msg = auditLog.GetStatus().GetMessage() + } + return nil, client.ErrDeploymentFailed{Message: msg} }src/pkg/clouds/gcp/cloudbuild.go (2)
62-86: Last tag wins when multiple non-defang-cdtags are present.The
Parsemethod iterates through all tags but overwritesStack,Project,Service, andEtagfor each matching tag. If multiple metadata tags exist (unlikely but possible), only the last one is retained. If this is intentional, consider adding a comment. Otherwise, consider returning after the first valid metadata tag is parsed.
106-111: Consider stricter validation forBuildTagfields.Line 107 checks if any of
Project,Service, orEtagis non-empty. This could return a partially populatedBuildTag. If all three fields are expected to be present for proper operation, consider using&&instead of||, or validate each field explicitly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/stream_test.gosrc/pkg/clouds/gcp/cloudbuild.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream_test.gosrc/pkg/cli/client/byoc/gcp/byoc_test.go
🧬 Code graph analysis (4)
src/pkg/cli/client/byoc/gcp/byoc_test.go (1)
src/protos/io/defang/v1/fabric.pb.go (3)
LogEntry(3460-3470)LogEntry(3483-3483)LogEntry(3498-3500)
src/pkg/cli/client/byoc/gcp/stream.go (3)
src/pkg/clouds/gcp/cloudbuild.go (1)
BuildTag(46-52)src/protos/io/defang/v1/fabric.pb.go (17)
Service(3835-3840)Service(3853-3853)Service(3868-3870)SubscribeResponse(4202-4211)SubscribeResponse(4224-4224)SubscribeResponse(4239-4241)Status(3248-3253)Status(3266-3266)Status(3281-3283)ServiceState(135-135)ServiceState(199-201)ServiceState(203-205)ServiceState(212-214)ServiceState_BUILD_ACTIVATING(143-143)ServiceState_BUILD_FAILED(151-151)ServiceState_BUILD_STOPPING(145-145)ServiceState_BUILD_RUNNING(144-144)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
src/pkg/cli/client/byoc/gcp/byoc.go (2)
src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/clouds/gcp/cloudbuild.go (1)
CloudBuildArgs(32-44)
src/pkg/clouds/gcp/cloudbuild.go (5)
src/pkg/logs/fluent.go (1)
Source(3-3)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/clouds/gcp/project.go (2)
Gcp(98-101)ProjectId(15-15)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
src/pkg/cli/client/byoc/gcp/stream_test.go (1)
56-67: LGTM!The change to return a slice of pointers (
[]*loggingpb.LogEntry) aligns with the updatedMockGcpLoggingListerinterface and is consistent with how protobuf messages are typically handled in Go.src/pkg/cli/client/byoc/gcp/byoc_test.go (1)
76-87: LGTM!The change from value slice to pointer slice is correct and consistent with how protobuf messages are typically handled.
src/pkg/cli/client/byoc/gcp/byoc.go (5)
335-340: LGTM!The
CloudBuildStepstruct is well-defined with appropriate yaml tags for Cloud Build step serialization.
448-450: LGTM!Clean simplification from Cloud Run execution polling to Cloud Build status polling, consistent with the new architecture.
377-379: LGTM!Correct conditional propagation of the etag to the
DEFANG_ETAGenvironment variable.
185-186: LGTM!Adding
roles/logging.logWriteris necessary for Cloud Build to write logs when using a custom service account.
409-416: No action needed. The tag format handling correctly supports emptyPulumiStackvalues. TheBuildTag.Parsefunction accepts both 3-part and 4-part tag formats (lines 73–83 insrc/pkg/clouds/gcp/cloudbuild.go), and an empty first segment (from_project_cd_etagformat) is parsed correctly as an empty Stack value. TheString()method explicitly handles this case by outputting the 3-part format when Stack is empty, confirming that empty stack values are intentional and supported.Likely an incorrect or invalid review comment.
src/pkg/cli/client/byoc/gcp/stream.go (3)
496-511: LGTM!Good consolidation of completion response generation. The helper correctly emits deployment-completed states for all ready services plus the CD entry.
705-729: LGTM!The non-CD build state handling correctly differentiates build lifecycle phases and includes appropriate nil checks for status extraction.
608-610: LGTM!The CD job success handling correctly uses
getReadyServicesCompletedRespsto emit completion states for all services.src/pkg/clouds/gcp/cloudbuild.go (2)
183-202: LGTM!The polling logic correctly handles the three states: success (
io.EOF), failure (ErrDeploymentFailed), and in-progress (nil). Usingio.EOFfor success aligns with the stream-based consumption pattern used elsewhere in the codebase.
222-236: LGTM!The GCS URI parsing correctly handles bucket/object extraction and properly unescapes URL-encoded characters in the object path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/pkg/cli/client/byoc/gcp/byoc.go:
- Around line 448-450: GetDeploymentStatus should validate b.cdExecution before
calling the driver: if b.cdExecution is empty, return an appropriate error
instead of calling b.driver.GetBuildStatus; otherwise call
b.driver.GetBuildStatus(ctx, b.cdExecution). Mirror the existing pattern used
elsewhere (see checks for b.cdExecution != "" near runCdCommand usages) and
ensure the returned error message clearly states that no CD execution ID is
available.
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/gcp/byoc.go (1)
399-416: Cloud Build execution is well-structured with minor tag format consideration.The Cloud Build integration correctly marshals steps and propagates configuration. The tagging strategy includes both specific identification (with stack, project, and etag) and a general "defang-cd" marker.
Note: When
cmd.etagis empty, the tag at line 413 will have a trailing underscore (e.g., "stack_project_cd_"). This is likely intentional for consistency, but consider whether an empty etag should use a different tag format or skip that component.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/cli/client/byoc/gcp/byoc_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/cli/client/byoc/gcp/byoc.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/byoc/gcp/byoc.go
🧬 Code graph analysis (1)
src/pkg/cli/client/byoc/gcp/byoc.go (2)
src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/clouds/gcp/cloudbuild.go (1)
CloudBuildArgs(32-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (7)
src/pkg/cli/client/byoc/gcp/byoc.go (7)
35-35: LGTM: YAML library choice is consistent with the codebase.The use of
gopkg.in/yaml.v3aligns with the existing codebase pattern and is appropriate for marshaling CloudBuildStep structs.
185-185: LGTM: Logging role addition aligns with Cloud Build migration.Adding
roles/logging.logWriteris necessary for Cloud Build to write logs and is appropriately scoped for the CD service account.
330-330: LGTM: Etag field addition supports deployment tracking.The etag field enables deployment state tracking through the Cloud Build workflow.
335-340: LGTM: CloudBuildStep struct design is sound.The struct correctly uses YAML tags to marshal steps for Cloud Build execution. Based on learnings, this intermediate-struct pattern aligns with the protobuf BuildStep fields through case-insensitive YAML unmarshaling.
377-379: LGTM: Etag propagation is correctly implemented.The conditional addition of
DEFANG_ETAGensures the etag is propagated to Cloud Build only when available.
394-397: LGTM: Environment variable slice construction is correct.The conversion from map to
[]stringin "KEY=VALUE" format matches the CloudBuildStep.Env field requirements.
503-509: LGTM: Etag propagation in deploy flow is correct.The etag is properly generated and propagated through the cdCommand for deployment operations, ensuring consistent state tracking.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/pkg/clouds/gcp/cloudbuild.go:
- Around line 106-110: The call to bt.Parse(build.Tags) ignores its error;
capture the returned error from bt.Parse and return it (wrapped with context)
instead of proceeding with potentially invalid data: replace the bare
bt.Parse(build.Tags) with something like err := bt.Parse(build.Tags) and if err
!= nil return nil, fmt.Errorf("failed to parse build tags %v: %w", build.Tags,
err), then keep the existing check for bt.Project/bt.Service/bt.Etag and the
final error return.
🧹 Nitpick comments (6)
src/pkg/clouds/gcp/cloudbuild.go (6)
37-37: Consider implementing the ETAG optimization.The TODO suggests using ETAG from object metadata to determine when a new build is necessary. This could reduce unnecessary builds and improve efficiency.
Do you want me to generate an implementation proposal or open an issue to track this optimization?
62-86: Clarify behavior with multiple valid tags.When multiple tags match the
stack_project_service_etagorproject_service_etagformat, each iteration overwrites the previous values (lines 74-83). Only the last valid tag's data will be retained in the BuildTag struct.Additionally, if an invalid tag is encountered after processing valid tags, the method returns an error but leaves the struct partially populated with data from earlier iterations.
Consider one of these approaches:
- Document that only the last valid tag is retained (if this is intentional).
- Return an error on the first invalid tag before processing any (to avoid partial state).
- Return an error if more than one valid deployment tag is found (if uniqueness is expected).
139-141: Consider making logging and timeout configurable.The logging option (line 139) and timeout (line 141) are hardcoded. While reasonable defaults, different build scenarios might benefit from different settings:
- Some builds may need longer than 1 hour
- Some users might prefer
LEGACYorGCS_ONLYlogging🔎 Suggested enhancement
Add optional fields to
CloudBuildArgs:type CloudBuildArgs struct { // Required fields Source string Steps string // TODO: We should be able to use ETAG from object metadata as ditest in Diff func to determine a new build is necessary Images []string `pulumi:"images,optional" provider:"replaceOnChanges"` ServiceAccount *string `pulumi:"serviceAccount,optional" provider:"replaceOnChanges"` Tags []string `pulumi:"tags,optional"` MachineType *string `pulumi:"machineType,optional"` DiskSizeGb *int64 `pulumi:"diskSizeGb,optional"` Substitutions map[string]string `pulumi:"substitutions,optional"` + Timeout *time.Duration `pulumi:"timeout,optional"` + Logging *cloudbuildpb.BuildOptions_LoggingMode `pulumi:"logging,optional"` }Then use them with defaults in
RunCloudBuild:+ timeout := time.Hour + if args.Timeout != nil { + timeout = *args.Timeout + } + logging := cloudbuildpb.BuildOptions_CLOUD_LOGGING_ONLY + if args.Logging != nil { + logging = *args.Logging + } + // Create a build request build := &cloudbuildpb.Build{ Substitutions: args.Substitutions, Steps: steps, // TODO: Support NPM or Python packages using Artifacts field // AvailableSecrets: secrets, Options: &cloudbuildpb.BuildOptions{ MachineType: GetMachineType(args.MachineType), DiskSizeGb: GetDiskSize(args.DiskSizeGb), - Logging: cloudbuildpb.BuildOptions_CLOUD_LOGGING_ONLY, + Logging: logging, }, - Timeout: durationpb.New(time.Hour), + Timeout: durationpb.New(timeout), Tags: args.Tags, }
125-128: Implement secrets support for secure environment variables.The TODOs indicate that secrets support is needed for passing environment variables securely to build steps. This is essential for builds that require credentials or sensitive configuration.
Do you want me to generate an implementation for Cloud Build secrets integration using Secret Manager, or open an issue to track this feature?
183-202: Document the return value semantics.The method has three return states that may not be immediately clear to callers:
io.EOFsignals successful build completion (line 197)ErrDeploymentFailedsignals build failure (line 199)nilwhenbuild == nil(line 201) - presumably means "build in progress, poll again"The use of
io.EOFto signal success is unconventional outside of reader interfaces. While this might be part of an intentional polling pattern, the semantics should be documented.Add a function comment documenting the return values:
// GetBuildStatus polls a Cloud Build operation and returns its status. // Returns: // - io.EOF: build completed successfully // - ErrDeploymentFailed: build failed // - nil: build is still in progress (caller should poll again) // - other error: API or infrastructure error func (gcp Gcp) GetBuildStatus(ctx context.Context, startBuildOpName string) error {Alternatively, consider using a more conventional pattern:
- Return a custom sentinel error for "in progress" instead of nil
- Return
(status, error)tuple instead of overloading error for state
222-237: Consider validating non-empty object path.The function correctly parses
gs://URIs and handles URL-encoded characters. However, it doesn't validate whether the object path is empty. URIs likegs://bucket/would result in an empty string for the object, which might be invalid for Cloud Build's StorageSource.Add a check after the split:
parts := strings.SplitN(uri[5:], "/", 2) if len(parts) < 2 { return "", "", errors.New("URI must contain a bucket and an object path") } obj, err := url.QueryUnescape(parts[1]) // Because the base 64 encoding may contain '=' if err != nil { return "", "", fmt.Errorf("failed to unescape object path: %w", err) } + if obj == "" { + return "", "", errors.New("object path cannot be empty") + } return parts[0], obj, nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pkg/clouds/gcp/cloudbuild.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
🧬 Code graph analysis (1)
src/pkg/clouds/gcp/cloudbuild.go (4)
src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/clouds/gcp/project.go (2)
Gcp(98-101)ProjectId(15-15)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
src/pkg/clouds/gcp/cloudbuild.go (4)
1-17: LGTM!The imports are well-organized and appropriate for Cloud Build integration. The use of
goccy/go-yamlaligns with the team's stated direction for protobuf YAML unmarshaling.
19-30: LGTM!The type alias and constants provide a clean, user-friendly API surface for machine types and build tagging.
46-60: LGTM!The BuildTag struct and String method correctly handle both legacy 3-part and current 4-part tag formats.
204-220: LGTM!The helper functions correctly handle optional parameters with sensible defaults (UNSPECIFIED for machine type, 0 for disk size).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/pkg/clouds/gcp/cloudbuild.go:
- Around line 185-204: The GetBuildStatus method uses
svc.CreateBuildOperation(...).Poll which can return (nil, nil) for ongoing
operations; replace the single-call op.Poll(ctx) with op.Wait(ctx) so the call
blocks until completion, then check the returned build and its Status (still
using Gcp.GetBuildStatus, svc.CreateBuildOperation and the op variable); remove
the current branch that returns nil when build is nil and instead handle the
final build result (success => return io.EOF, non-success => return
client.ErrDeploymentFailed with the formatted status) and propagate any error
from op.Wait(ctx).
🧹 Nitpick comments (5)
src/pkg/clouds/gcp/cloudbuild.go (4)
62-86: Consider clarifying the behavior when multiple build info tags are present.If
tagscontains multiple non-DefangCD tags (e.g.,["stack1_proj1_svc1_etag1", "stack2_proj2_svc2_etag2"]), the method will overwrite theBuildTagfields with each subsequent tag, keeping only the last one. If only one build info tag is expected per build, this is fine. Otherwise, consider either:
- Returning an error when multiple build info tags are detected, or
- Documenting that only the last tag is used
143-143: Consider making the timeout configurable.The build timeout is hardcoded to
time.Hour. For long-running builds or quick builds, a configurable timeout viaCloudBuildArgswould provide more flexibility.🔎 Suggested enhancement
Add a
Timeoutfield toCloudBuildArgs:type CloudBuildArgs struct { // Required fields Source string Steps string + Timeout *time.Duration `pulumi:"timeout,optional"` Images []string `pulumi:"images,optional" provider:"replaceOnChanges"` ServiceAccount *string `pulumi:"serviceAccount,optional" provider:"replaceOnChanges"` // ... rest of fields }Then use it with a default:
+ timeout := time.Hour + if args.Timeout != nil { + timeout = *args.Timeout + } build := &cloudbuildpb.Build{ // ... - Timeout: durationpb.New(time.Hour), + Timeout: durationpb.New(timeout), }
172-176: Remove or update the misleading comment.Line 173 states "Replace with your GCP project ID" but the code already uses
gcp.ProjectId. Either remove the comment or clarify it (e.g., "Uses the GCP project ID from the receiver").🔎 Proposed fix
// Trigger the build op, err := client.CreateBuild(ctx, &cloudbuildpb.CreateBuildRequest{ - ProjectId: gcp.ProjectId, // Replace with your GCP project ID + ProjectId: gcp.ProjectId, // Current API endpoint does not support location // Parent: fmt.Sprintf("projects/%s/locations/%s", args.ProjectId, args.Location), Build: build, })
224-239: Clarify the comment and validate non-empty object names.Two issues:
Misleading comment (line 233): The comment states "Because the base 64 encoding may contain '='", but
url.QueryUnescapedecodes URL percent-encoding (e.g.,%20for space), not base64. If the object path is percent-encoded (common in URLs), this is correct. Consider updating the comment to:// Unescape percent-encoded characters in the object path.Empty object name validation: The current validation allows URIs like
gs://bucket/where the object name is an empty string. If Cloud Build requires a non-empty object name, add a check after line 236:🔎 Proposed fixes
} - obj, err := url.QueryUnescape(parts[1]) // Because the base 64 encoding may contain '=' + obj, err := url.QueryUnescape(parts[1]) // Unescape percent-encoded characters in the object path if err != nil { return "", "", fmt.Errorf("failed to unescape object path: %w", err) } + if obj == "" { + return "", "", errors.New("object path cannot be empty") + } return parts[0], obj, nil }src/go.mod (1)
42-42: Update to a newer version ofgithub.com/goccy/go-yaml.Version v1.17.1 is outdated. The latest stable release is v1.19.1 (released 17 Dec 2025), with intermediate releases v1.18.0 and v1.19.0 also available. While v1.17.1 has no known security vulnerabilities, consider upgrading to v1.19.1 to benefit from recent improvements and maintenance.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/go.modsrc/pkg/clouds/gcp/cloudbuild.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/go.modsrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T03:07:48.228Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:48.228Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
🧬 Code graph analysis (1)
src/pkg/clouds/gcp/cloudbuild.go (2)
src/pkg/clouds/gcp/project.go (2)
Gcp(98-101)ProjectId(15-15)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (6)
src/pkg/clouds/gcp/cloudbuild.go (6)
3-17: LGTM!All added imports are necessary and properly utilized throughout the file.
19-44: LGTM!The type alias, constants, and
CloudBuildArgsstruct are well-designed. The use of pointers for optional fields and the clear separation of required vs. optional fields enhances API clarity.
46-52: LGTM!The addition of the
IsDefangCDfield cleanly extends theBuildTagstruct to support the new Defang CD tagging feature.
88-113: LGTM!The error handling for
bt.Parse()has been correctly implemented, addressing the previous review concern. The method now properly validates and returns errors for invalid tag formats.
206-215: LGTM!The helper correctly handles nil and invalid machine type values by defaulting to
UNSPECIFIED.
217-222: LGTM!The helper correctly returns a default value (0) when the disk size is not specified, allowing Cloud Build to use its default.
edwardrf
force-pushed
the
edw/gcp-run-cd-in-cloudbuild
branch
from
73101c6 to
526109e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/pkg/cli/client/byoc/gcp/byoc.go:
- Line 30: The import path used ("go.yaml.in/yaml/v3") is incorrect; replace it
with the documented standard "gopkg.in/yaml.v3" wherever it appears (e.g., the
import block in byoc.go and other files referencing the YAML v3 package) so the
code imports the official YAML v3 package; update any go files' import
statements to use "gopkg.in/yaml.v3" and run `go mod tidy` / rebuild to verify
compilation.
In @src/pkg/cli/client/byoc/gcp/stream.go:
- Around line 497-512: The getReadyServicesCompletedResps function shadows its
parameter status with the map loop variable; rename the loop variable (e.g.,
svcStatus or rsStatus) used in for serviceName, svcStatus := range readyServices
so the parameter status remains distinct and then use svcStatus for each
readyServices entry while keeping the final append for defangCD using the
original parameter status.
- Around line 704-705: The return currently calls
getReadyServicesCompletedResps(auditLog.GetStatus().String()) without guarding
against a nil status; change it to first retrieve status :=
auditLog.GetStatus(), set a fallback like statusStr := "UNKNOWN" (or the same
fallback used at the pattern on line 714) and if status != nil then set
statusStr = status.String(), then return
getReadyServicesCompletedResps(statusStr), nil; keep the cdSuccess = true
assignment as-is and use auditLog.GetStatus() and getReadyServicesCompletedResps
to locate the change.
- Around line 700-702: The error path uses auditLog.GetStatus().GetMessage()
without guarding against a nil Status; update the block where entry.Severity ==
logtype.LogSeverity_ERROR to first check if auditLog.GetStatus() != nil and use
auditLog.GetStatus().GetMessage() when present, otherwise pass a sensible
fallback string (e.g., "deployment failed" or auditLog.String()) into
client.ErrDeploymentFailed{Message: ...} so the error message is never derived
from a nil Status.
In @src/pkg/cli/common.go:
- Line 11: Replace the non-standard import path "go.yaml.in/yaml/v3" with the
correct Go YAML v3 module path "gopkg.in/yaml.v3" in the import block (where the
current import statement for YAML appears) so the package resolves correctly
during build.
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/gcp/stream.go (1)
454-461: Consider a more robust CD start detection mechanism.The string prefix matching (
" ** "or"Defang: ") to detect CD start is fragile and could break if log formats change. While the HACK comment indicates awareness, consider whether:
- A more reliable signal exists (e.g., specific log labels or structured fields)
- The consequences of false positives/negatives are acceptable
If this is temporary, consider adding a TODO with a tracking issue for a more robust solution.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/go.modsrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/query.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.querysrc/pkg/cli/client/byoc/gcp/testdata/with_everything.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_run.querysrc/pkg/cli/client/byoc/gcp/testdata/with_project.querysrc/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.querysrc/pkg/cli/common.gosrc/pkg/cli/compose/loader.gosrc/pkg/clouds/aws/ecs/cfn/template_test.go
✅ Files skipped from review due to trivial changes (2)
- src/pkg/cli/compose/loader.go
- src/pkg/clouds/aws/ecs/cfn/template_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/go.mod
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
📚 Learning: 2026-01-07T03:07:48.228Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:48.228Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.go
📚 Learning: 2026-01-07T17:31:18.174Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:185-204
Timestamp: 2026-01-07T17:31:18.174Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the GetBuildStatus method is intentionally designed as a non-blocking status check. It uses op.Poll(ctx) rather than op.Wait(ctx), and returns nil when the build is nil (operation still in progress), allowing the caller to control the polling loop. The method should not be changed to use Wait() as the waiting logic is handled by the caller.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.go
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/common.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/query.gosrc/pkg/cli/common.go
🧬 Code graph analysis (2)
src/pkg/cli/client/byoc/gcp/stream.go (3)
src/pkg/clouds/gcp/cloudbuild.go (1)
BuildTag(46-52)src/protos/io/defang/v1/fabric.pb.go (14)
Service(3835-3840)Service(3853-3853)Service(3868-3870)SubscribeResponse(4202-4211)SubscribeResponse(4224-4224)SubscribeResponse(4239-4241)ServiceState_DEPLOYMENT_COMPLETED(149-149)Status(3248-3253)Status(3266-3266)Status(3281-3283)ServiceState(135-135)ServiceState(199-201)ServiceState(203-205)ServiceState(212-214)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
src/pkg/cli/client/byoc/gcp/byoc.go (4)
src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/clouds/gcp/cloudbuild.go (1)
CloudBuildArgs(32-44)src/pkg/cli/compose/loader.go (2)
Project(25-25)Services(29-29)src/pkg/logs/log_type.go (2)
LogType(8-8)LogTypeRun(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (22)
src/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.query (3)
12-12: LGTM: Stack-based filtering additions are consistent.The addition of
defang-stack = "beta"labels across cloud_run_job, cloud_run_revision, and gce_instance resources provides consistent stack-based scoping for the query, aligning with the Cloud Build CD changes.Also applies to: 19-19, 22-22
15-15: LGTM: Build tags pattern correctly includes stack prefix.The pattern update from
__[A-Za-z0-9-]{1,63}_tobeta__[a-zA-Z0-9-]{1,63}_appropriately adds the stack name prefix for more targeted build tag filtering.
16-16: LGTM: MAIN build step exclusion aligns with PR objectives.The addition of
-labels.build_step="MAIN"correctly filters out Cloud Build MAIN step logs, directly implementing the PR objective to exclude logs such as "DONE".src/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.query (1)
9-14: LGTM! Test data correctly reflects the new query pattern.The additions align with the PR's Cloud Build integration changes:
- Stack-based filtering with
defang-stack = "beta"- Updated build tag pattern to include stack prefix
- MAIN step exclusion to reduce noise
src/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.query (1)
9-23: LGTM! Consistent stack filtering across resource types.The testdata correctly applies
defang-stack = "beta"filtering across all resource types (cloud_run_job, cloud_run_revision, gce_instance) and updates the build tag pattern with the stack prefix, consistent with the broader query changes in this PR.src/pkg/cli/client/byoc/gcp/testdata/with_etag.query (1)
9-23: LGTM! Etag-based query correctly updated.The testdata properly incorporates stack filtering and MAIN step exclusion while maintaining etag-based filtering across all resource types.
src/pkg/cli/client/byoc/gcp/query.go (1)
164-165: LGTM! MAIN step exclusion reduces Cloud Build log noise.The exclusion of MAIN build step logs (FETCHSOURCE, PUSH, DONE) is appropriate for filtering out system-level build operations and focusing on application-specific logs.
src/pkg/cli/client/byoc/gcp/testdata/with_everything.query (1)
12-28: LGTM!The addition of
defang-stack = "beta"labels across resource types and the updated build tag pattern with MAIN step exclusion are consistent with the Cloud Build-based CD changes. The query structure correctly filters logs for the new tagging scheme.src/pkg/cli/client/byoc/gcp/testdata/with_logtype_run.query (1)
8-13: LGTM!The
defang-stack = "beta"label additions forcloud_run_revisionandgce_instanceresource types are consistent with the broader labeling changes in this PR.src/pkg/cli/client/byoc/gcp/testdata/with_project.query (1)
8-23: LGTM!The labeling and build tag pattern updates are consistent with other testdata files in this PR.
src/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.query (1)
8-23: LGTM!The build tag pattern
beta__[a-zA-Z0-9-]{1,63}_test-etagcorrectly reflects the etag-only filtering scenario (double underscore indicates empty project field).src/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.query (1)
11-23: LGTM!The CD execution query correctly adds the
defang-stack = "beta"labeling alongside the existing execution name filter, with proper MAIN step exclusion for build logs.src/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.query (1)
7-20: LGTM!The "all" log type query correctly incorporates both build and run resource labeling with the new
defang-stack = "beta"filter.src/pkg/cli/client/byoc/gcp/byoc.go (7)
335-340: LGTM!The
CloudBuildStepstruct is well-designed as an intermediate type for YAML marshaling. Based on learnings, this pattern intentionally uses lowercase YAML keys that match the protobufBuildStepfield names via case-insensitive matching during unmarshaling incloudbuild.go.
394-408: LGTM!The environment variable handling and YAML marshaling are correctly implemented. The conversion from map to slice produces a non-deterministic order, but this doesn't affect Cloud Build execution.
409-424: LGTM!The Cloud Build invocation properly tags builds for log filtering. The tag format
{stack}_{project}_cd_{etag}aligns with the testdata query patterns, and thedefang-cdtag enables identification of CD-specific builds.
449-451: LGTM!The simplified
GetDeploymentStatuscorrectly delegates todriver.GetBuildStatus. Based on learnings, the lack of pre-validation forb.cdExecutionis intentional—empty values will surface as GCP API errors.
504-510: LGTM!The etag is correctly propagated to the
cdCommandstruct for inclusion in Cloud Build environment variables.
579-584: LGTM!The log query methods now correctly pass
b.PulumiStackto enable thedefang-stacklabel filtering introduced across the PR.
185-186: LGTM!The
roles/logging.logWriterrole is correctly added to enable Cloud Build log writing for the CD service account.src/pkg/cli/client/byoc/gcp/stream.go (2)
6-6: LGTM! Good improvements to error handling and log parsing.The additions are well-structured:
- Import of
fmtfor error wrapping provides better error context.- The
cdStartedflag correctly persists across log entries to skip initial cloudbuild setup logs.- Build tags parsing safely checks for empty string before splitting.
- Error wrapping at line 429 improves debugging experience.
Also applies to: 382-382, 403-408, 429-429
707-731: LGTM! Well-structured build state tracking.The state machine for non-CD builds is clear and comprehensive:
- Proper handling of build lifecycle stages using
Operation.FirstandOperation.Last- Appropriate state transitions (ACTIVATING → RUNNING → STOPPING/FAILED)
- Good defensive coding with nil check at line 714
- Fallback status handling when status is empty
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @src/pkg/cli/client/byoc/gcp/stream.go:
- Line 705: The code calls auditLog.GetStatus().String() which can panic if
GetStatus() returns nil; update the return to first guard against a nil status
(e.g., assign status := "" then if s := auditLog.GetStatus(); s != nil { status
= s.String() }) and pass that safe status string into
getReadyServicesCompletedResps instead of calling .String() directly on a
possibly nil value.
- Around line 700-702: The error return for Cloud Deployment failures calls
auditLog.GetStatus().GetMessage() without guarding against auditLog.GetStatus()
being nil, which can panic; update the branch where entry.Severity ==
logtype.LogSeverity_ERROR to first fetch the status, check it for nil, extract
Message only if non-nil (e.g., msg := ""; if st := auditLog.GetStatus(); st !=
nil { msg = st.GetMessage() }), then return client.ErrDeploymentFailed{Message:
msg} to avoid nil pointer dereference.
- Around line 497-512: The helper getReadyServicesCompletedResps shadows its
parameter status with the for-loop variable, causing the defangCD response to
pick a map-iteration value; rename the loop variable (e.g., svcStatus) and use
that in the loop-created SubscribeResponse entries, and ensure the final
appended SubscribeResponse for defangCD continues to use the original function
parameter status so the CD completion status is deterministic.
In @src/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.query:
- Line 13: The regex for labels.build_tags currently ends with an extra
underscore and won't match actual build tags; update the pattern used in the
query (refer to labels.build_tags =~ "beta_test-project_[a-zA-Z0-9-]{1,63}_") to
remove the trailing underscore and include the etag segment instead (for example
change to a pattern like "beta_test-project_[a-zA-Z0-9-]{1,63}_[a-zA-Z0-9-]+" or
another pattern matching the real etag format).
In @src/pkg/clouds/gcp/cloudbuild.go:
- Around line 224-239: The parseGCSURI function currently uses url.QueryUnescape
which will turn '+' into space and corrupt object names; change it to
url.PathUnescape to preserve '+' and other path characters, and add a check that
the bucket (parts[0]) is non-empty before returning; keep the existing error
wrapping on unescape failures and return the bucket and unescaped object via the
same return values.
- Around line 115-183: The comment for CloudBuildArgs.Source is inaccurate: it
currently says "Required" but RunCloudBuild treats Source as optional (checked
with if args.Source != "") and the caller in byoc.go never supplies it; update
the CloudBuildArgs.Source struct field comment to indicate that Source is
optional (e.g., "Optional GCS URI for build source; if empty, no storage source
is set") or remove the "Required" label and document expected format/behavior so
it matches RunCloudBuild and callers like byoc.go.
🧹 Nitpick comments (2)
src/pkg/clouds/gcp/cloudbuild.go (1)
206-215: Don’t silently accept invalidmachineTypestrings. Falling back toUNSPECIFIEDmakes misconfigurations hard to detect. Prefer returning an error (or at least logging/warning) when the value is unknown.src/pkg/cli/client/byoc/gcp/stream.go (1)
454-461: Consider documenting the expected CD output format for maintainability.The heuristic relies on specific prefixes (
" ** "and"Defang: ") to detect CD start. If the CD image output format changes, this detection could break silently. Consider adding a reference to where these prefixes are defined in the CD codebase or using a more explicit marker.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/go.modsrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/client/byoc/gcp/query.gosrc/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/stream_test.gosrc/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.querysrc/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.querysrc/pkg/cli/client/byoc/gcp/testdata/with_everything.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.querysrc/pkg/cli/client/byoc/gcp/testdata/with_logtype_run.querysrc/pkg/cli/client/byoc/gcp/testdata/with_project.querysrc/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.querysrc/pkg/cli/common.gosrc/pkg/cli/compose/loader.gosrc/pkg/clouds/aws/ecs/cfn/template_test.gosrc/pkg/clouds/gcp/cloudbuild.go
🚧 Files skipped from review as they are similar to previous changes (8)
- src/pkg/cli/client/byoc/gcp/query.go
- src/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.query
- src/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.query
- src/pkg/cli/common.go
- src/pkg/cli/compose/loader.go
- src/pkg/cli/client/byoc/gcp/stream_test.go
- src/pkg/cli/client/byoc/gcp/byoc_test.go
- src/pkg/clouds/aws/ecs/cfn/template_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
📚 Learning: 2026-01-07T03:07:48.228Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:48.228Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T17:31:18.174Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:185-204
Timestamp: 2026-01-07T17:31:18.174Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the GetBuildStatus method is intentionally designed as a non-blocking status check. It uses op.Poll(ctx) rather than op.Wait(ctx), and returns nil when the build is nil (operation still in progress), allowing the caller to control the polling loop. The method should not be changed to use Wait() as the waiting logic is handled by the caller.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/go.modsrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/clouds/gcp/cloudbuild.go
🧬 Code graph analysis (3)
src/pkg/cli/client/byoc/gcp/stream.go (4)
src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/clouds/gcp/cloudbuild.go (1)
BuildTag(46-52)src/protos/io/defang/v1/fabric.pb.go (17)
Service(3943-3948)Service(3961-3961)Service(3976-3978)SubscribeResponse(4310-4319)SubscribeResponse(4332-4332)SubscribeResponse(4347-4349)Status(3348-3353)Status(3366-3366)Status(3381-3383)ServiceState(135-135)ServiceState(199-201)ServiceState(203-205)ServiceState(212-214)ServiceState_BUILD_ACTIVATING(143-143)ServiceState_BUILD_FAILED(151-151)ServiceState_BUILD_STOPPING(145-145)ServiceState_BUILD_RUNNING(144-144)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
src/pkg/cli/client/byoc/gcp/byoc.go (5)
src/pkg/types/etag.go (1)
ETag(9-9)src/protos/io/defang/v1/fabric.pb.go (4)
DeploymentMode(83-83)DeploymentMode(118-120)DeploymentMode(122-124)DeploymentMode(131-133)src/pkg/term/colorizer.go (1)
Debugf(298-300)src/pkg/clouds/gcp/cloudbuild.go (1)
CloudBuildArgs(32-44)src/pkg/logs/log_type.go (2)
LogType(8-8)LogTypeRun(20-20)
src/pkg/clouds/gcp/cloudbuild.go (2)
src/pkg/clouds/gcp/project.go (2)
Gcp(98-101)ProjectId(15-15)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (22)
src/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.query (2)
9-9: LGTM: Consistent stack label filtering added.The addition of
defang-stack = "beta"labels across cloud_run_job, cloud_run_revision, and gce_instance resource types is consistent and aligns with the PR objective to include stack-based filtering in queries.Also applies to: 17-17, 21-21
14-14: LGTM: MAIN step exclusion aligns with PR objectives.The addition of
-labels.build_step="MAIN"correctly excludes Cloud Build MAIN step logs (such as "DONE"), which matches the PR objective to filter out these logs.src/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.query (4)
12-12: LGTM: Stack-based filtering added consistently.The
defang-stack = "beta"label filter has been correctly added across cloud_run_job, cloud_run_revision, and gce_instance resource types, aligning with the PR's shift to stack-based query filtering.Also applies to: 19-19, 22-22
15-15: LGTM: Build tag pattern now stack-scoped.The regex pattern correctly scopes build tags to the "beta" stack, matching the stack-based filtering strategy introduced in this PR.
1-24: Test coverage status unclear.This testdata file has been updated to reflect the new stack-based filtering and MAIN step exclusion. However, the PR checklist indicates that appropriate tests have not been added yet. Please ensure that the test coverage adequately validates:
- Cloud Build-based CD execution
- Stack-based filtering across all resource types
- MAIN step log exclusion logic
16-16: Syntax is valid. The-labels.build_step="MAIN"negation syntax is correct in Google Cloud Logging and properly excludes MAIN build steps.src/pkg/cli/client/byoc/gcp/testdata/with_project.query (1)
8-15: Scoped filters + MAIN-step exclusion look consistent with the new Cloud Build CD log model.Also applies to: 16-23
src/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.query (1)
8-15: LGTM: defang-stack scoping + MAIN-step exclusion align with the updated tag/label semantics.Also applies to: 16-23
src/pkg/cli/client/byoc/gcp/testdata/with_everything.query (1)
11-19: LGTM: this keeps “everything” scoped to the stack while avoiding noisy MAIN build-step logs.Also applies to: 20-29
src/pkg/cli/client/byoc/gcp/testdata/with_logtype_run.query (1)
8-13: LGTM: adds stack scoping to the logtype=run query branches.src/pkg/cli/client/byoc/gcp/testdata/with_etag.query (1)
8-15: LGTM: stack scoping + MAIN-step exclusion for build logs is consistent with the new tagging.Also applies to: 16-23
src/pkg/clouds/gcp/cloudbuild.go (1)
32-44: Avoid echoing fullargs.Stepson YAML parse errors (potential secret leakage). Today the error wraps and prints the entire steps YAML.Proposed fix
- if err := yaml.Unmarshal([]byte(args.Steps), &steps); err != nil { - return "", fmt.Errorf("failed to parse cloudbuild steps: %w, steps are:\n%v\n", err, args.Steps) - } + if err := yaml.Unmarshal([]byte(args.Steps), &steps); err != nil { + // Avoid leaking any env values that may be embedded in steps YAML. + return "", fmt.Errorf("failed to parse cloudbuild steps: %w", err) + }Also applies to: 115-126
⛔ Skipped due to learnings
Learnt from: edwardrf Repo: DefangLabs/defang PR: 1747 File: src/pkg/clouds/gcp/cloudbuild.go:120-123 Timestamp: 2026-01-06T20:32:17.173Z Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.Learnt from: edwardrf Repo: DefangLabs/defang PR: 1747 File: src/pkg/clouds/gcp/cloudbuild.go:185-204 Timestamp: 2026-01-07T17:31:18.174Z Learning: In src/pkg/clouds/gcp/cloudbuild.go, the GetBuildStatus method is intentionally designed as a non-blocking status check. It uses op.Poll(ctx) rather than op.Wait(ctx), and returns nil when the build is nil (operation still in progress), allowing the caller to control the polling loop. The method should not be changed to use Wait() as the waiting logic is handled by the caller.src/go.mod (1)
39-45: The YAML dependencies are not consolidable as suggested.go.yaml.in/yaml/v3andgopkg.in/yaml.v3are different packages with distinct import paths (not versions of the same library). The former is used directly in 5+ files, while the latter is only an indirect transitive dependency fromcompose-spec/compose-go/v2. Additionally,github.com/goccy/go-yamlserves a specific, intentional purpose for CloudBuild protobuf unmarshaling and cannot be replaced by the standard YAML libraries.Likely an incorrect or invalid review comment.
src/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.query (1)
1-21: LGTM!The query updates correctly add stack-based label filtering and exclude Cloud Build MAIN step logs, which aligns with the new Cloud Build-based CD execution flow.
src/pkg/cli/client/byoc/gcp/stream.go (2)
382-382: ThecdStartedflag persists across multiple log parsing calls, which is intentional but requires awareness.The closure captures
cdStartedingetLogEntryParser, meaning once CD logs start, all subsequent logs pass through. This works correctly for a single stream session but note that a new parser is created per stream.
406-408: LGTM!Good defensive check - guarding against empty
build_tagsbefore splitting prevents parsing issues with a single-element slice containing an empty string.src/pkg/cli/client/byoc/gcp/byoc.go (6)
335-340: LGTM!The
CloudBuildStepstruct correctly uses lowercase YAML tags that enable case-insensitive matching when unmarshaling into the protobufBuildSteptype. Based on learnings, this intermediate-struct pattern is intentional.
399-408: LGTM!The Cloud Build step is correctly marshaled to YAML with the CD image, command arguments, and environment variables. This aligns with the expected Cloud Build step format.
449-451: LGTM!The switch to
GetBuildStatusfor deployment status checking aligns with the Cloud Build-based execution model. Based on learnings, this is a non-blocking status check where the caller controls the polling loop.
579-584: LGTM!The log queries now consistently use
b.PulumiStackfor stack-based filtering, which aligns with the new Cloud Build tagging and labeling scheme.
185-186: LGTM!Adding
roles/logging.logWriteris necessary for Cloud Build to emit logs. This is a required permission for the new CD execution model.
413-416: Tag format is compatible withBuildTag.Parse.The tag format
{stack}_{project}_cd_{etag}correctly matches the parsing logic and will be parsed as Stack, Project="cd", Service, Etag. The "defang-cd" literal tag properly sets the IsDefangCD flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/pkg/cli/client/byoc/gcp/stream.go (2)
454-461: Fragile log prefix detection heuristic.The
cdStarteddetection relies on specific log message prefixes (" ** "or"Defang: "). If the CD image's log output format changes, this heuristic could break silently, causing all CD logs to be skipped.Consider defining these prefixes as constants or adding a fallback mechanism if neither prefix is detected after a reasonable number of log entries.
700-713: Inconsistent status extraction between error and success paths.The error path (line 703) uses
GetMessage()to extract a human-readable message, while the success path (line 711) usesString()which produces a full protobuf string representation (e.g.,code:0 message:"OK"). This could result in inconsistent status formats displayed to users.Consider using
GetMessage()consistently for both paths if a human-readable status is preferred.♻️ Proposed fix for consistent status extraction
cdSuccess = true status := "" if auditLog.GetStatus() != nil { - status = auditLog.GetStatus().String() + status = auditLog.GetStatus().GetMessage() } return getReadyServicesCompletedResps(status), nilsrc/pkg/clouds/gcp/cloudbuild.go (3)
174-174: Remove the placeholder comment.The comment "Replace with your GCP project ID" should be removed as
gcp.ProjectIdis correctly populated from the receiver.♻️ Proposed fix
op, err := client.CreateBuild(ctx, &cloudbuildpb.CreateBuildRequest{ - ProjectId: gcp.ProjectId, // Replace with your GCP project ID + ProjectId: gcp.ProjectId, // Current API endpoint does not support location // Parent: fmt.Sprintf("projects/%s/locations/%s", args.ProjectId, args.Location), Build: build,
116-184: Consider adding validation for required fields.The method currently doesn't validate that
args.Stepsis non-empty before attempting to unmarshal. While the unmarshal would fail anyway, an explicit check with a clearer error message would improve debuggability.♻️ Proposed validation
func (gcp Gcp) RunCloudBuild(ctx context.Context, args CloudBuildArgs) (string, error) { + if args.Steps == "" { + return "", errors.New("Steps is required") + } + client, err := cloudbuild.NewClient(ctx) if err != nil { return "", fmt.Errorf("failed to create Cloud Build client: %w", err)
237-237: Clarify the comment about path unescaping.The comment states "Because the base 64 encoding may contain '='", but
url.PathUnescapeis for URL percent-encoding, not base64. The unescaping may still be necessary for GCS object names that contain URL-encoded characters, but the comment incorrectly attributes this to base64 encoding.♻️ Proposed fix
- obj, err := url.PathUnescape(parts[1]) // Because the base 64 encoding may contain '=' + obj, err := url.PathUnescape(parts[1]) // GCS object names may contain URL-encoded characters if err != nil { return "", "", fmt.Errorf("failed to unescape object path: %w", err)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:62-86
Timestamp: 2026-01-09T20:18:59.234Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, BuildTag.Parse should fail on unexpected tag formats (tags that don't have 3-4 underscore-separated parts or aren't DefangCDBuildTag) because build tags are strictly controlled and only created in two places: (1) running CD in cloudbuild by CLI, and (2) building images by CD. Unexpected tags indicate an error case.
📚 Learning: 2026-01-09T20:31:15.468Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/stream.go:497-512
Timestamp: 2026-01-09T20:31:15.468Z
Learning: Pattern: Apply to all Go files under the byoc/gcp client code. Enriched instruction: In Go, avoid variable shadowing when iterating maps or ranges. In getReadyServicesCompletedResps, the loop variable derived from the readyServices map should not shadow the function parameter that represents the CD service completion status. Use distinct names (for example, use serviceStatus for the map value and cdStatus for the function parameter) and preserve each ready service's original status from the map. This prevents overwriting the parameter and ensures per-service statuses are kept intact during processing.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.go
📚 Learning: 2026-01-09T20:18:59.234Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:62-86
Timestamp: 2026-01-09T20:18:59.234Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, BuildTag.Parse should fail on unexpected tag formats (tags that don't have 3-4 underscore-separated parts or aren't DefangCDBuildTag) because build tags are strictly controlled and only created in two places: (1) running CD in cloudbuild by CLI, and (2) building images by CD. Unexpected tags indicate an error case.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.go
📚 Learning: 2026-01-07T03:07:48.228Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:48.228Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T17:31:18.174Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:185-204
Timestamp: 2026-01-07T17:31:18.174Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the GetBuildStatus method is intentionally designed as a non-blocking status check. It uses op.Poll(ctx) rather than op.Wait(ctx), and returns nil when the build is nil (operation still in progress), allowing the caller to control the polling loop. The method should not be changed to use Wait() as the waiting logic is handled by the caller.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-09T20:12:21.986Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:30-30
Timestamp: 2026-01-09T20:12:21.986Z
Learning: In Go files, recognize and accept the import path go.yaml.in/yaml/v3 as the maintained fork of the YAML library. Do not flag this import as incorrect; this fork supersedes the archived gopkg.in/yaml.v3 path. If you encounter this or similar forked import paths, treat them as valid Go imports and do not raise review flags.
Applied to files:
src/pkg/cli/client/byoc/gcp/stream.gosrc/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-09T20:18:59.234Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:62-86
Timestamp: 2026-01-09T20:18:59.234Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, enforce BuildTag.Parse to fail for any tag format that is not strictly controlled: require that tags have exactly 3–4 underscore-separated parts and that the tag is DefangCDBuildTag (or a clearly defined equivalent type/constant). This tag format check should be performed during parsing to catch unexpected, potentially erroneous tags, since build tags are only created in two controlled paths: (1) running CD in Cloud Build via CLI, and (2) building images by CD. Add unit tests covering valid cases (defang CD build tags) and invalid cases (wrong number of parts, non-DefangCDBuildTag), and fail fast with a descriptive error message when the constraint is violated.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
📚 Learning: 2026-01-09T20:31:15.468Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/stream.go:497-512
Timestamp: 2026-01-09T20:31:15.468Z
Learning: In src/pkg/cli/client/byoc/gcp/stream.go, the getReadyServicesCompletedResps helper function intentionally uses variable shadowing. The loop variable `status` from `readyServices` map represents individual service statuses, while the function parameter (to be renamed `cdStatus`) represents only the CD service (defangCD) completion status. Each ready service should retain its original status from the map.
Applied to files:
src/pkg/clouds/gcp/cloudbuild.go
🧬 Code graph analysis (1)
src/pkg/clouds/gcp/cloudbuild.go (2)
src/pkg/clouds/gcp/project.go (2)
Gcp(98-101)ProjectId(15-15)src/pkg/cli/client/errors.go (1)
ErrDeploymentFailed(5-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (16)
src/pkg/cli/client/byoc/gcp/stream.go (6)
382-382: LGTM!The
cdStartedflag is correctly scoped within the closure and will persist across log entries, enabling the intended behavior of skipping cloud build image-pulling logs before CD starts.
403-408: LGTM!Correct handling of
build_tagsparsing with the empty string guard to prevent a single-element slice containing an empty string.
429-429: LGTM!Good improvement to error context using
fmt.Errorfwith%wfor proper error wrapping.
497-512: LGTM!The helper correctly consolidates the completion response logic. Variable naming (
statusfor map value,cdStatusfor parameter) appropriately avoids shadowing. Based on learnings, this pattern correctly preserves each ready service's original status from the map.
714-739: LGTM with minor note.The state machine logic for non-CD builds is clear and correctly handles the three phases (activating, running, stopping/failed). The fallback to
state.String()ensures there's always a status.Note: Line 723 has the same
.String()vs.GetMessage()inconsistency mentioned above.
611-611: LGTM!Correctly uses the new helper to emit completion responses for ready services.
src/pkg/clouds/gcp/cloudbuild.go (10)
3-17: LGTM!The new imports appropriately support the Cloud Build orchestration functionality. The YAML library import path is the maintained fork, which is correct.
19-28: LGTM!The type alias and constants provide a clean public API for machine types while properly mapping to the underlying protobuf values.
30-30: LGTM!The constant clearly identifies Defang CD builds in the tagging system.
32-45: LGTM!The struct design provides a clear configuration interface for Cloud Build. The Pulumi tags are appropriate for infrastructure-as-code integration.
47-61: LGTM!The addition of the
IsDefangCDfield enables proper tracking of Defang CD builds, complementing the metadata tag format in theString()method.
63-87: Verify the behavior with multiple non-DefangCD tags.The current implementation iterates through all tags but would silently overwrite BuildTag fields if multiple non-DefangCD metadata tags are present (only the last one would be retained). Based on learnings, tags are strictly controlled and created in only two places, suggesting this scenario shouldn't occur. However, if it does happen (e.g., due to a bug or API change), the silent overwriting might mask an error condition.
Consider whether detecting and rejecting multiple metadata tags would provide better error detection.
89-114: LGTM!The method correctly handles the updated
Parsesignature and appropriately validates that build information was found in the tags before returning the BuildTag.
186-205: LGTM!The implementation correctly uses
Pollfor non-blocking status checks and appropriately returnsio.EOFon success,ErrDeploymentFailedon failure, andnilwhen the build is still in progress. This design allows the caller to control the polling loop as intended.Based on learnings, this non-blocking pattern is the correct design for this method.
207-223: LGTM!The helper functions correctly handle optional parameters by returning sensible defaults (UNSPECIFIED for machine type, 0 for disk size) when values are nil.
225-246: LGTM!The GCS URI parsing includes comprehensive validation for the URI format, bucket name, and object path. The error messages are clear and specific.
Description
For a faster cd start up in GCP, cd boots up in ~15s instead of 1.5m when running on cloud run.
Linked Issues
https://github.com/DefangLabs/defang-mvp/issues/2556
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.