Add subscribe API support for azure#2118
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Azure BYOC Subscribe: cloud helpers to read ACA revisions and ACR runs, ByocAzure cdStart tracking, a Subscribe implementation that multiplexes per-service revision and build pollers into an iterator, and integration/unit tests for mappings and polling. ChangesAzure Deployment Event Streaming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/azure/subscribe_test.go (1)
251-276: ⚡ Quick winCollapse
mapRevisionStateunit tests into a table-driven test.These three tests are repetitive and should be table-driven for consistency and easier case expansion.
As per coding guidelines:
**/*_test.go: Use table-driven tests for comprehensive test coverage in unit tests.🤖 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/subscribe_test.go` around lines 251 - 276, Replace the three repetitive tests (TestMapRevisionState_NotFound, TestMapRevisionState_Healthy, TestMapRevisionState_FailedProvisioning) with a single table-driven test that iterates a slice of cases (each with a name, input *aca.RevisionState, expected defangv1.ServiceState value, and expected terminal bool), calling mapRevisionState for each case inside t.Run; include the three cases: nil → UPDATE_QUEUED/terminal=false, &aca.RevisionState{NotFound:true} → UPDATE_QUEUED/terminal=false, healthyRevision() → DEPLOYMENT_COMPLETED/terminal=true, and &aca.RevisionState{ProvisioningState: armappcontainersv3.RevisionProvisioningStateFailed} → DEPLOYMENT_FAILED/terminal=true, and assert expected state and terminal for each case.
🤖 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/subscribe_test.go`:
- Line 230: The test currently ignores the error returned by drain when calling
got, _ := drain(t, t.Context(), subscribeInputs{...}); update the call in the
build-state test to capture and assert the error (e.g., err := drain(...)) and
fail the test when err != nil using the test helper (t.Fatal/t.Fatalf or the
test harness’s equivalent). Ensure you reference the drain function and
subscribeInputs invocation so the test fails on any stream/drain errors instead
of silently passing.
In `@src/pkg/cli/client/byoc/azure/subscribe.go`:
- Around line 139-144: The loop in subscribe.go currently treats ev.err
non-terminal by calling yield(nil, ev.err) and then continuing; change the
control flow so that after emitting an error via yield (when ev.err != nil) the
subscription loop breaks/returns immediately instead of continuing, ensuring
pollers stop and the iterator completes; locate the block handling ev.err in the
subscription loop (the section that calls yield(nil, ev.err)) and replace the
continue with a terminal exit (return or break) so errors are treated as
terminal by the Subscribe/iterator logic.
In `@src/pkg/clouds/azure/aca/revisions.go`:
- Around line 24-27: Wrap the raw errors returned from the credential/setup
calls with contextual messages using fmt.Errorf and %w: replace direct returns
of err from the c.NewCreds() call (the cred, err := c.NewCreds() path) and the
other setup return around the 35-38 block with something like fmt.Errorf("setup
<describe operation>: %w", err) so callers see which operation failed; add the
fmt import if missing and ensure the message names the operation (e.g.,
"creating Azure creds" or "initializing subscription").
In `@src/pkg/clouds/azure/acr/runs.go`:
- Around line 38-41: Wrap and annotate raw errors returned from setup/discovery
paths so callers keep context: in the functions/methods ensureClients (where you
call r.NewCreds()), findRegistry, and the ListRunsSince entry path, replace bare
"return err" with fmt.Errorf including a short contextual message and %w to wrap
the original error (e.g., "ensureClients: failed to get creds: %w"). Use
fmt.Errorf consistently for each failing call site referenced (including the
spots around the r.NewCreds() call and the other locations at the review comment
ranges) so the error chain preserves original error values while adding
operation context.
- Around line 109-117: The early-exit check uses a zero-valued create when
run.Properties.CreateTime is nil, causing create.Before(since) to be true and
prematurely stopping pagination; modify the logic in the block handling
run.Properties.CreateTime so the "if !since.IsZero() && create.Before(since)"
check only runs when run.Properties.CreateTime != nil (i.e., move the since
comparison inside the CreateTime non-nil branch or otherwise skip the
early-return when CreateTime is nil) to avoid skipping newer runs.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/azure/subscribe_test.go`:
- Around line 251-276: Replace the three repetitive tests
(TestMapRevisionState_NotFound, TestMapRevisionState_Healthy,
TestMapRevisionState_FailedProvisioning) with a single table-driven test that
iterates a slice of cases (each with a name, input *aca.RevisionState, expected
defangv1.ServiceState value, and expected terminal bool), calling
mapRevisionState for each case inside t.Run; include the three cases: nil →
UPDATE_QUEUED/terminal=false, &aca.RevisionState{NotFound:true} →
UPDATE_QUEUED/terminal=false, healthyRevision() →
DEPLOYMENT_COMPLETED/terminal=true, and &aca.RevisionState{ProvisioningState:
armappcontainersv3.RevisionProvisioningStateFailed} →
DEPLOYMENT_FAILED/terminal=true, and assert expected state and terminal for each
case.
🪄 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: 89863da9-0416-4dd9-a371-2c745552b027
📒 Files selected for processing (6)
src/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/azure/byoc_test.gosrc/pkg/cli/client/byoc/azure/subscribe.gosrc/pkg/cli/client/byoc/azure/subscribe_test.gosrc/pkg/clouds/azure/aca/revisions.gosrc/pkg/clouds/azure/acr/runs.go
lionello
approved these changes
May 22, 2026
CD success/failure is already detected by WaitForCdTaskExit in
TailAndMonitor, which calls ByocAzure.GetDeploymentStatus on a separate
goroutine and cancels svcStatusCtx on failure. Tracking CD state inside
Subscribe meant we emitted the same failure twice and gated revision-
completed events on a signal the caller already observes independently.
Removes pollCD, cdGate, the pendingReady gating in the aggregator, the
post-CD deadline in pollRevision, and allServicesTerminal. Flattens
subscribeEvent into a plain chan since pollCD was the only emitter of
the err and cdSucceeded fields.
The producer no longer self-terminates; cleanup relies on the consumer
cancelling the parent ctx (TailAndMonitor does this on command exit).
Documented in the Subscribe godoc.
Smoke-tested on real Azure: happy path, CD failure, and revision failure
all behave correctly. The CD-failure path now surfaces solely via
WaitForCdTaskExit → ErrDeploymentFailed{Message: "CD job ...: Failed"}
with Pulumi context preserved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/azure/subscribe_test.go (1)
70-89: ⚡ Quick winLet the test harness assert stream errors explicitly.
drainhard-fails on the first iterator error, so this suite still can't cover the non-NotFoundprovider failures fromGetRevisionState/ListRunsSince. Returning the terminal error would make it possible to add the missing provider-error cases for this new path.As per coding guidelines, "Add tests for new behavior and important failure modes: not-found vs other errors, duplicate adds, missing returns, blocking behavior, provider mismatches, and optional-file handling".
🤖 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/subscribe_test.go` around lines 70 - 89, The test helper drain currently calls subscribe(ctx, in) and t.Fatalf on the first iterator error, preventing tests from asserting terminal stream errors; change drain to return ([]*defangv1.SubscribeResponse, error) instead of failing: iterate over subscribe(ctx, in), collect responses into got, and if the iterator yields a non-nil err return got, err (or nil if iteration completed normally); update the drain signature and all test call sites to expect and assert the returned error so provider failures from GetRevisionState/ListRunsSince can be explicitly asserted; keep references to subscribe, subscribeInputs, and defangv1.SubscribeResponse when making these changes.
🤖 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/subscribe.go`:
- Around line 99-119: The iterator currently drops all errors because eventCh is
typed as chan *defangv1.SubscribeResponse and pollBuilds/pollRevision only log
failures; change the design so errors are delivered to the caller: either
convert eventCh to carry a union (e.g., a struct with Response
*defangv1.SubscribeResponse and Err error) or add a separate errCh that
pollBuilds and pollRevision can send real errors on; update pollBuilds,
pollRevision, the anonymous wg.Wait goroutine, and the loop that reads from
eventCh to handle received errors (wrap and return them instead of retrying/only
logging), and ensure proper closing of channels and wg usage so errors are
propagated out of the subscribe iterator via yield/return rather than swallowed.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/azure/subscribe_test.go`:
- Around line 70-89: The test helper drain currently calls subscribe(ctx, in)
and t.Fatalf on the first iterator error, preventing tests from asserting
terminal stream errors; change drain to return ([]*defangv1.SubscribeResponse,
error) instead of failing: iterate over subscribe(ctx, in), collect responses
into got, and if the iterator yields a non-nil err return got, err (or nil if
iteration completed normally); update the drain signature and all test call
sites to expect and assert the returned error so provider failures from
GetRevisionState/ListRunsSince can be explicitly asserted; keep references to
subscribe, subscribeInputs, and defangv1.SubscribeResponse when making these
changes.
🪄 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: 57db65d4-284e-4d09-8df5-9f94670ea950
📒 Files selected for processing (2)
src/pkg/cli/client/byoc/azure/subscribe.gosrc/pkg/cli/client/byoc/azure/subscribe_test.go
The 16-slot buffer was a holdover from the pre-refactor multi-channel design. All sends already select on ctx.Done(), so an unbuffered channel is correct, and at this volume (5s tick, a few producers) the latency difference is unobservable.
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
So defang cli only exits when all services are up correctly.
Linked Issues
#2073
Checklist
Summary by CodeRabbit
New Features
Tests