Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a new LogTypeCD and updates log-tailing to treat CD logs separately across the CLI and BYOC clients; AWS BYOC log querying now derives task IDs for CD etags and tails logs by task ID; minor GCP and test adjustments and a user-facing error message change in DNS delegation set creation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant BYOC_AWS
participant ECS_CloudWatch
CLI->>BYOC_AWS: QueryLogs(request with etag, logType)
BYOC_AWS->>BYOC_AWS: deriveTaskID(etag, cdTaskArn)
alt etag refers to CD task
BYOC_AWS->>ECS_CloudWatch: Tail logs by taskID (follow/range)
ECS_CloudWatch-->>BYOC_AWS: Stream log events
BYOC_AWS-->>CLI: Forward aggregated log events
else legacy or non-CD
BYOC_AWS->>ECS_CloudWatch: Query logs by etag/stream filters
ECS_CloudWatch-->>BYOC_AWS: Return matched logs
BYOC_AWS-->>CLI: Return results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/logs/log_type.go (1)
26-30:⚠️ Potential issue | 🟡 MinorInclude
LogTypeCDinAllLogTypes.
String()and CLI help text depend onAllLogTypes. WithoutLogTypeCDthere, combined values likeLogTypeBuild | LogTypeCDstringify incompletely and--typehelp omitsCD.🔧 Proposed fix
var AllLogTypes = []LogType{ LogTypeRun, LogTypeBuild, + LogTypeCD, LogTypeAll, }Also applies to: 82-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/logs/log_type.go` around lines 26 - 30, AllLogTypes currently omits LogTypeCD causing String() and CLI help to miss CD; add LogTypeCD to the AllLogTypes slice (alongside LogTypeRun, LogTypeBuild, LogTypeAll) and update the other AllLogTypes-like collection referenced later in the file (the alternate slice around the LogType definitions used for String()/help generation) so combined values like LogTypeBuild | LogTypeCD stringify and appear in --type help correctly.
🧹 Nitpick comments (2)
src/pkg/cli/getServices_test.go (1)
390-392: Use a reserved invalid TLD for deterministic network-failure tests.
invalid-endpointcan resolve on some networks via search domains.invalid.invalidis guaranteed invalid.🔧 Suggested tweak
- endpoint: "https://invalid-endpoint", + endpoint: "https://invalid.invalid",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/getServices_test.go` around lines 390 - 392, Replace the non-deterministic test endpoint used in the "Invalid endpoint" test case: update the endpoint value referenced by the test case (the endpoint field in the failing case named "Invalid endpoint" in getServices_test) from "https://invalid-endpoint" to a reserved invalid TLD such as "https://invalid.invalid" so DNS resolution cannot succeed on some networks and the network-failure test is deterministic.src/pkg/logs/log_type_test.go (1)
33-45: Add a combined CD case to lock compositeString()behavior.Single-value coverage is good; adding a combo case prevents regressions in bitfield formatting.
✅ Suggested test addition
{ {"unspecified", LogTypeUnspecified, "UNSPECIFIED"}, {"cd", LogTypeCD, "CD"}, {"run", LogTypeRun, "RUN"}, {"build", LogTypeBuild, "BUILD"}, + {"build and cd", LogTypeBuild | LogTypeCD, "BUILD,CD"}, {"run and build", LogTypeRun | LogTypeBuild, "RUN,BUILD"}, {"all", LogTypeAll, "ALL"}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/logs/log_type_test.go` around lines 33 - 45, The test suite TestLogTypeString lacks a composite case that includes the CD bit, so add a combined-case entry to the tests slice to verify bitfield formatting for composites (e.g., add an entry like {"cd+run", LogTypeCD | LogTypeRun, "CD,RUN"}) to ensure LogType.String() correctly joins multiple flags; update the tests slice in TestLogTypeString to include this new test case alongside the existing ones.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/estimate.go`:
- Around line 72-75: GeneratePreview currently tails only CD logs (TailOptions
with LogType set to logs.LogTypeCD), which misses preview output classified as
build logs; update GeneratePreview to tail both Build and CD logs — either by
changing TailOptions.LogType to include both (e.g., logs.LogTypeAll or a
combined value) or by invoking the tailing call twice with TailOptions using
logs.LogTypeBuild and logs.LogTypeCD (matching the approach in preview.go) so
all preview-related messages are captured; modify the code that constructs
TailOptions and/or the tail invocation inside GeneratePreview to ensure both log
types are processed.
In `@src/pkg/logs/log_type.go`:
- Around line 18-23: The LogType constants have wrong numeric values and
All/AllLogTypes omits LogTypeCD; update the LogType declarations so their
numeric values match the protobuf (explicitly assign LogTypeCD=1,
LogTypeBuild=2, LogTypeRun=4) and restore LogTypeUnspecified=0, then define
LogTypeAll (or AllLogTypes) as the bitwise OR of LogTypeCD, LogTypeBuild and
LogTypeRun so LogTypeCD is included in all-string/iteration logic; touch the
constants block that declares LogTypeUnspecified, LogTypeCD, LogTypeRun,
LogTypeBuild and the LogTypeAll/AllLogTypes definition to apply these changes.
---
Outside diff comments:
In `@src/pkg/logs/log_type.go`:
- Around line 26-30: AllLogTypes currently omits LogTypeCD causing String() and
CLI help to miss CD; add LogTypeCD to the AllLogTypes slice (alongside
LogTypeRun, LogTypeBuild, LogTypeAll) and update the other AllLogTypes-like
collection referenced later in the file (the alternate slice around the LogType
definitions used for String()/help generation) so combined values like
LogTypeBuild | LogTypeCD stringify and appear in --type help correctly.
---
Nitpick comments:
In `@src/pkg/cli/getServices_test.go`:
- Around line 390-392: Replace the non-deterministic test endpoint used in the
"Invalid endpoint" test case: update the endpoint value referenced by the test
case (the endpoint field in the failing case named "Invalid endpoint" in
getServices_test) from "https://invalid-endpoint" to a reserved invalid TLD such
as "https://invalid.invalid" so DNS resolution cannot succeed on some networks
and the network-failure test is deterministic.
In `@src/pkg/logs/log_type_test.go`:
- Around line 33-45: The test suite TestLogTypeString lacks a composite case
that includes the CD bit, so add a combined-case entry to the tests slice to
verify bitfield formatting for composites (e.g., add an entry like {"cd+run",
LogTypeCD | LogTypeRun, "CD,RUN"}) to ensure LogType.String() correctly joins
multiple flags; update the tests slice in TestLogTypeString to include this new
test case alongside the existing ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1825afd0-a3b1-467a-a5b0-d2251c6b1cb9
📒 Files selected for processing (11)
src/cmd/cli/command/compose.gosrc/pkg/cli/cd.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/domain.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/estimate.gosrc/pkg/cli/getServices_test.gosrc/pkg/cli/preview.gosrc/pkg/logs/log_type.gosrc/pkg/logs/log_type_test.go
edwardrf
approved these changes
Mar 6, 2026
Contributor
edwardrf
left a comment
edwardrf
left a comment
There was a problem hiding this comment.
Verified to have fixed the bug on aws and works on gcp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #1961, reapplying #1960
The previous PR fixed the panic but regressed by only ever showing CD logs, because the
QueryLogscode can't tell whether the caller wants all logs or only CD/task logs. To fix this, I added a separate LogType for CD.Summary by CodeRabbit
New Features
Bug Fixes
Tests