Always use etag for gcp cdCommands, remove useless code

[edwardrf]

Description

• Remove GetCDExecutionContext as it is not actually useful, when to stop querying cd logs is controlled by WaitForCdTaskExit by calling provider.GetDeploymentStatus()
• Return ETag from gcp CdCommand, since we can find the cd logs from cloudbuild using etags, thus no need to overload etag with cd execution id, this fixes the bug where the suggested tail command after a ctrl+c interrupts a defang cd command fails due to the deployment id is not a valid etag for gcp.

Checklist

• I have performed a self-review of my code
• I have added appropriate tests
• I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

• Refactor
  • Simplified deployment status and log tracking by transitioning from execution ID-based to etag-based identification
  • Updated logging filter queries to streamline resource matching and reduce unnecessary constraints in Cloud Logging

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors the GCP BYOC CD flow to track deployments using etag-based identifiers instead of CD execution names. The CdCommand method signature changes to return types.ETag, internal runCdCommand logic is simplified, and log/status queries are updated to remove cloud_run_job resource filtering and execution-name label matching.

Changes

Cohort / File(s)	Summary
Core Logic Updates src/pkg/cli/client/byoc/gcp/byoc.go	Removed cdEtag field from ByocGcp struct; added etag field to cdCommand struct; updated CdCommand to return types.ETag instead of string; simplified runCdCommand to return error only; removed CD execution tracking and getCDExecutionContext logic; shifted Subscribe/Log handling to PulumiStack-based status updates instead of cdExecution-specific paths.
Test Updates src/pkg/cli/client/byoc/gcp/byoc_test.go	Removed TestGetCDExecutionContext; updated TestSetUpCD to discard first return value from runCdCommand; removed two test cases from TestGetLogStream: with_cd_exec and with_etag_equal_cd_exec.
Query Testdata Simplifications src/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.query, with_etag.query, with_etag_and_since.query, with_etag_equal_cd_exec.query, with_everything.query, with_logtype_all.query, with_logtype_build.query, with_project.query, with_project_since_and_until.query	Consistently removed cloud_run_job resource type filtering, execution_name label matching, and build_step="MAIN" constraints across multiple testdata query files; narrowed filter scope to build, cloud_run_revision, and gce_instance resources.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Run GCP cd in gcp cloud build #1747: Modifies the same GCP BYOC CD flow, including etag field additions to cdCommand and updates to log/status query paths to use etag and PulumiStack instead of CD execution tracking.

Suggested reviewers

• lionello
• jordanstephens

Poem

🐰 The execution fades, etag takes the stage,
Cloud logs now simpler on the page,
Build steps and runners all align,
With Pulumi stacks, the traces shine! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: switching GCP cdCommands to always use etag and removing unnecessary code (GetCDExecutionContext and related logic).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46e2f78 and e414121.

📒 Files selected for processing (11)

• src/pkg/cli/client/byoc/gcp/byoc.go
• src/pkg/cli/client/byoc/gcp/byoc_test.go
• src/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.query
• src/pkg/cli/client/byoc/gcp/testdata/with_etag.query
• src/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.query
• src/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.query
• src/pkg/cli/client/byoc/gcp/testdata/with_everything.query
• src/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.query
• src/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.query
• src/pkg/cli/client/byoc/gcp/testdata/with_project.query
• src/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.query

💤 Files with no reviewable changes (9)

• src/pkg/cli/client/byoc/gcp/testdata/with_etag_equal_cd_exec.query
• src/pkg/cli/client/byoc/gcp/testdata/with_everything.query
• src/pkg/cli/client/byoc/gcp/testdata/with_cd_exec.query
• src/pkg/cli/client/byoc/gcp/testdata/with_project_since_and_until.query
• src/pkg/cli/client/byoc/gcp/testdata/with_logtype_all.query
• src/pkg/cli/client/byoc/gcp/testdata/with_etag_and_since.query
• src/pkg/cli/client/byoc/gcp/testdata/with_etag.query
• src/pkg/cli/client/byoc/gcp/testdata/with_logtype_build.query
• src/pkg/cli/client/byoc/gcp/testdata/with_project.query

🧰 Additional context used

🧠 Learnings (8)

📓 Common learnings

Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:56.002Z
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.

Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/stream.go:497-512
Timestamp: 2026-01-09T20:31:23.614Z
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.

📚 Learning: 2026-01-07T03:07:56.002Z

Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:56.002Z
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/byoc_test.go
• src/pkg/cli/client/byoc/gcp/byoc.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: 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/byoc_test.go
• 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_test.go
• src/pkg/cli/client/byoc/gcp/byoc.go

📚 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/byoc_test.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/byoc_test.go
• src/pkg/cli/client/byoc/gcp/byoc.go

📚 Learning: 2026-01-09T20:19:04.424Z

Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:62-86
Timestamp: 2026-01-09T20:19:04.424Z
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/byoc.go

📚 Learning: 2026-01-07T17:31:23.220Z

Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:185-204
Timestamp: 2026-01-07T17:31:23.220Z
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/byoc.go

🧬 Code graph analysis (1)

src/pkg/cli/client/byoc/gcp/byoc.go (3)

src/pkg/types/etag.go (1)

• NewEtag (11-13)

src/pkg/cli/compose/loader.go (2)

• Project (25-25)
• Services (29-29)

src/pkg/cli/client/byoc/common.go (1)

• GetPulumiBackend (34-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). (1)

• GitHub Check: Analyze (go)

🔇 Additional comments (8)

src/pkg/cli/client/byoc/gcp/byoc_test.go (2)

42-44: LGTM!

The test correctly adapts to the updated runCdCommand signature that now returns only error instead of (string, error). This aligns with the PR's goal of simplifying the CD command flow.

---

103-163: Test coverage looks good for the etag-centric approach.

The test cases properly cover various combinations of log query parameters with etags. The retained cdExecution field in the test struct (line 107) and its usage in with_everything case (line 161) is appropriate since the ByocGcp.cdExecution field still exists and affects query generation.

src/pkg/cli/client/byoc/gcp/byoc.go (6)

311-328: LGTM!

The refactored CdCommand method correctly generates the etag upfront and returns it on success. This ensures consistent etag tracking for CD log queries, addressing the bug mentioned in the PR where the suggested tail command failed because deployment ID wasn't a valid GCP ETag.

---

347-437: LGTM!

The simplified runCdCommand signature returning only error is appropriate. The method still sets b.cdExecution internally (line 434) for use by GetDeploymentStatus, which aligns with the existing design where empty cdExecution produces an "invalid operation name" error from the GCP API. Based on learnings, this is the intended behavior.

---

525-530: LGTM!

The deployment flow correctly uses the locally-generated etag (line 478) in the CD command and returns it in the response. The simplification of error handling is clean.

---

543-544: LGTM!

The status update subscriptions now consistently use etag for filtering, aligning with the PR's objective to rely on etags for locating CD logs in Cloud Build.

---

566-571: LGTM!

The log stream now uses etag consistently for both build and run log types. This is the key change that ensures the tail command works correctly after Ctrl+C, since etag is a valid GCP identifier for locating Cloud Build logs.

---

724-733: LGTM!

The deployment log query now includes Compute Engine log queries (line 727) and instance group status updates (line 731), expanding observability coverage. The simplified comments and consistent etag-based filtering align with the PR's refactoring goals.

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"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}