Migrate predeploy/postdeploy to service-level event handlers in agents extension#8788
Conversation
Switch predeploy and postdeploy from WithProjectEventHandler to WithServiceEventHandler with Host filter (azure.ai.agent). This lets the core handle host-type filtering, removing the need for manual loops and checks in the handlers. The handler signature changes from ProjectEventArgs (all services) to ServiceEventArgs (single service), simplifying each handler to operate on args.Service directly. preprovision, postprovision, and postdown remain as project-level handlers because the core only raises those events on the project config. Fixes #7989 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
There was a problem hiding this comment.
Pull request overview
This PR updates the azure.ai.agents extension lifecycle hook wiring so predeploy and postdeploy run as service-level event handlers (filtered to Host: AiAgentHost) rather than project-level handlers that iterate and filter services manually. This reduces per-handler boilerplate and aligns with the core’s service hook model.
Changes:
- Switched
predeploy/postdeployregistration fromWithProjectEventHandlertoWithServiceEventHandlerwithServiceEventOptions{Host: AiAgentHost}. - Refactored
predeployHandlerandpostdeployHandlerto operate on a singleargs.Serviceinstead of looping overargs.Project.Services. - Updated tests to construct
azdext.ServiceEventArgsand validate the new “non-hosted agent” early-return behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go |
Registers predeploy/postdeploy as service hooks and simplifies handlers to per-service logic. |
cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go |
Adjusts unit tests to use ServiceEventArgs and reflect the updated postdeploy skip behavior. |
Comments suppressed due to low confidence (1)
cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go:168
- azd-code-reviewer: With service-level predeploy hooks, this handler can run once per agent service (potentially in parallel). Calling project.CheckDeveloperRBAC per hosted service will repeat Graph/ARM RBAC lookups and may also race on the optional auto-assign path, producing duplicated/noisy output and increasing throttling risk. Consider de-duplicating this pre-flight check so it runs at most once per azd deploy invocation (e.g., a package-level sync.Once guarding CheckDeveloperRBAC, or other per-run gating).
// Run developer RBAC pre-flight checks only for hosted agent deployments.
if isHostedAgentService(svc, args.Project) {
if err := project.CheckDeveloperRBAC(ctx, azdClient); err != nil {
return err
}
}
- Add sync.Once guard around CheckDeveloperRBAC in predeployHandler to avoid repeated ARM/Graph calls when multiple agent services are deployed - Remove duplicate TestPostdeployHandler_SkipsNonHostedAgentService (same coverage already exists in TestPostdeployHandler_NonHostedAgent_NoOp) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
There was a problem hiding this comment.
Clean simplification. The migration from project-level to service-level handlers removes unnecessary iteration and lets the core handle host-type filtering, which is the right architectural direction.
One concern with the sync.Once error propagation pattern in predeployHandler (see inline comment). Not blocking since the multi-hosted-agent-service + RBAC-failure scenario is uncommon, but worth addressing to keep the semantics correct.
Tests appropriately track the handler signature changes and drop coverage for filtering now handled by the core.
huimiu
left a comment
There was a problem hiding this comment.
Nice cleanup on this migration! One thing I want to flag so we don't lose it: the old reportOptimizationDeployments wrapped each service in a recover() to keep optimization reporting best-effort (per our "log failures but never block the deploy" contract). Calling reportSvcOptimizationDeployment directly drops that guard, and since the broker turns a recovered panic into a handler error, a panic here would now fail the whole deploy. What do you think about keeping the recover() — either at the call site or pushed into the func?
- Promote rbacErr to package-level developerRBACErr so all service handlers see the RBAC check failure, not just the first caller. - Wrap reportSvcOptimizationDeployment in defer/recover to maintain the best-effort contract (panics are logged, not propagated). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
There was a problem hiding this comment.
The third commit addresses both open concerns from earlier reviews.
developerRBACErr is now package-level, so all service handlers see the RBAC check failure, not just the first caller. This matches the suggestion from my earlier review and is correct.
The reportSvcOptimizationDeployment call is now wrapped in defer/recover(), restoring the best-effort contract that @huimiu flagged. Panics in optimization reporting won't block the deploy.
One note: reportOptimizationDeployments (the multi-service loop wrapper in optimize_helpers.go) is no longer called from production code. It's only exercised by its own tests. Consider removing it in a follow-up to avoid dead code.
@vhvb1989's approval was submitted against cadaedc4a and is now stale against the current HEAD 357ca6d8. Will need reapproval before merge.
Motivation
The agents extension uses
WithProjectEventHandlerfor all lifecycle events, which means each handler receives the full project config and must manually loop through services filtering bysvc.Host == AiAgentHost. This is error-prone and unnecessary for events that fire at the service level.Approach
Switch
predeployandpostdeployfromWithProjectEventHandlertoWithServiceEventHandlerwith&azdext.ServiceEventOptions{Host: AiAgentHost}. The core now handles host-type filtering automatically, and each handler receives a single*azdext.ServiceEventArgsinstead of needing to iterate all services.Key changes:
WithServiceEventHandlerwith the host filterpredeployHandler: Operates directly onargs.Service-- no loop, no host checkpostdeployHandler: ChecksisHostedAgentServiceon the single service, fetches agent version and sets up RBAC for that one service, callsreportSvcOptimizationDeploymentdirectlypreprovision,postprovision, andpostdownremain as project-level handlers because the core only raises those events on the project config (not on individual service configs).Notes
reportOptimizationDeployments(the multi-service wrapper) is no longer called frompostdeployHandlerbut remains inoptimize_helpers.gowith its dedicated testsCheckDeveloperRBACis idempotent so being called per-service (if multiple agent services exist) is safeFixes: #7989