Fix azure log query#2149
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughAdds a ChangesAzure log tailing: follow/snapshot mode, etag-based run discovery, build service dedup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.12.2)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." 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: 4
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/azure/byoc_test.go (1)
261-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen these tests to assert the intended etag-lookup failure path.
At Line 270 and Line 285, checking only
err != nilcan pass on unrelated failures. Assert the error contains the lookup context so regressions in etag-discovery behavior are caught.As per coding guidelines, “Test behavior boundaries, not implementation mechanics” and “Add tests for new behavior and important failure modes.”
💡 Proposed assertion tightening
_, err := b.QueryLogs(ctx, &defangv1.TailRequest{Etag: "some-etag"}) - if err == nil { - t.Error("QueryLogs should surface the lookup error when the run can't be discovered") + if err == nil || !strings.Contains(err.Error(), "failed to find CD deployment for etag") { + t.Errorf("expected etag lookup failure, got: %v", err) } @@ _, err := b.QueryLogs(ctx, &defangv1.TailRequest{Etag: "etag-B"}) - if err == nil { - t.Error("QueryLogs should look up the mismatched etag and surface the lookup error") + if err == nil || !strings.Contains(err.Error(), "failed to find CD deployment for etag") { + t.Errorf("expected mismatched-etag lookup failure, got: %v", err) }// add to imports import "strings"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/azure/byoc_test.go` around lines 261 - 286, The error assertions in TestQueryLogsDiscoversRunByEtag and TestQueryLogsEtagMismatchTriggersLookup are too weak—they only check if an error occurred but don't validate the error contains the expected lookup context. Add the strings package import to the test file, then strengthen both error checks to verify the error message contains the expected lookup failure details (such as the "denied" error from useFakeCred) rather than just asserting err != nil. This ensures the tests actually verify the etag-discovery behavior and catch regressions in how lookup errors are surfaced.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/cli/client/byoc/azure/byoc.go`:
- Around line 948-951: The code at line 948 where svc.Err is checked is silently
dropping Azure provider errors by only logging them at debug level and
continuing the loop. Instead of calling continue after the debug log, capture
these errors and return them wrapped with context about which service failed.
Apply the same fix to the similar error handling block at line 969 where
provider errors are also being silently swallowed. Both locations need to
surface the actual provider failures to the caller rather than allowing the
function to return an apparently successful but empty result.
In `@src/pkg/cli/tail.go`:
- Around line 164-173: The issue is that the code unconditionally trims the
service name suffix before checking if the service exists, which causes
incorrect warnings or deduping when the full name with suffix (like foo-image)
is an actual deployed service. Instead of trimming the suffix upfront, first
attempt to validate the exact service name using provider.GetService with the
original service value. Only if that returns a CodeNotFound error should you
then apply strings.TrimSuffix to get the base name and retry the GetService
lookup as a fallback. This ensures accurate service validation and proper
build-alias behavior on the second attempt.
In `@src/pkg/clouds/azure/aca/job.go`:
- Around line 578-580: The code iterates over Azure SDK slices without checking
if individual elements are nil before dereferencing them. In the loop iterating
over page.Value around line 578, add a nil check for exec itself before
accessing exec.Name or exec.Properties. Similarly, for the loops around lines
601-604 that iterate over containers and environment variables, add nil checks
for container and env var elements respectively before accessing their fields.
This prevents panic errors when the Azure SDK returns nil slice elements due to
partial or irregular API payloads.
In `@src/pkg/clouds/azure/acr/buildlogs.go`:
- Around line 149-154: In non-follow mode, the function returns early at the
!follow check but the goroutines started around line 140 continue executing
streamRunLog (lines 178-253) until runs reach a terminal status, causing
senders.Wait() to block indefinitely instead of returning promptly. To fix this,
use a context (such as ctx or a cancellation context) that gets cancelled when
the !follow condition is met, then pass this context to streamRunLog so those
goroutines will exit immediately rather than waiting for terminal status,
allowing senders.Wait() to complete and close the output channel without
hanging.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/azure/byoc_test.go`:
- Around line 261-286: The error assertions in TestQueryLogsDiscoversRunByEtag
and TestQueryLogsEtagMismatchTriggersLookup are too weak—they only check if an
error occurred but don't validate the error contains the expected lookup
context. Add the strings package import to the test file, then strengthen both
error checks to verify the error message contains the expected lookup failure
details (such as the "denied" error from useFakeCred) rather than just asserting
err != nil. This ensures the tests actually verify the etag-discovery behavior
and catch regressions in how lookup errors are surfaced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c62dc6ea-a3fb-4cbe-88f6-611dfb58f2f9
📒 Files selected for processing (14)
src/cmd/cli/command/compose.gosrc/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/azure/byoc_test.gosrc/pkg/cli/tail.gosrc/pkg/cli/tail_test.gosrc/pkg/clouds/azure/aca/job.gosrc/pkg/clouds/azure/aca/job_test.gosrc/pkg/clouds/azure/aca/tail.gosrc/pkg/clouds/azure/aca/tail_test.gosrc/pkg/clouds/azure/acr/buildlogs.gosrc/pkg/clouds/azure/acr/buildlogs_test.gosrc/pkg/clouds/azure/cd/driver_test.gosrc/pkg/clouds/azure/cd/setup.gosrc/pkg/logs/log_type.go
edwardrf
force-pushed
the
edw/fix-azure-log-query
branch
from
fbf8bf8 to
9b6e211
Compare
jordanstephens
approved these changes
Jun 24, 2026
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
4 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.
Description
azure
defang logdidn't work due to the logic requiring cd run id, updated to use etag to find the right run from azure.Checklist
Summary by CodeRabbit
New Features
Improvements
-imagebuild service suffixes.Tests