Repository navigation
Go codegen: address review feedback from #22057 - #22201
David Pine (IEvangelist) wants to merge 3 commits into
Conversation
- FormatError finds joined errors behind fmt.Errorf %w wrappers - DTO callbacks bind the registering client so tokens/handles work - Union parameters with handle members propagate Err() before sending - Forwarded callback tokens reuse the host-issued ID - Builder Err API summary reflects aggregated errors Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 22201Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 22201" |
github-actions
Bot
added
the
area-integrations
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical defects can let builds proceed after failed resource operations and cause cross-client cancellation to miss forwarded operations or target unrelated tokens.
3 open issues
What changed in this PR
This follow-up to #22057 improves error propagation and callback handling in Aspire’s generated Go SDK.
Changes:
- Formats wrapped aggregate errors and propagates failed handle arguments in unions.
- Binds DTO callbacks to their registering client and preserves host-issued cancellation IDs when forwarding tokens.
- Corrects the builder’s
Err()API summary and adds regression tests with updated snapshots.
| File | Description |
|---|---|
tests/Aspire.Hosting.CodeGeneration.Go.Tests/Snapshots/TwoPassScanningGeneratedAspire.verified.go |
Updates generated callback decoding and union error checks. |
tests/Aspire.Hosting.CodeGeneration.Go.Tests/Snapshots/GoApiExportTests.GoOnlySurface.verified.json |
Updates the builder’s aggregated-error summary. |
tests/Aspire.Hosting.CodeGeneration.Go.Tests/Snapshots/EscapedMetadataGeneratedAspire.verified.go |
Reflects client-bound DTO callbacks with escaped names. |
tests/Aspire.Hosting.CodeGeneration.Go.Tests/Snapshots/AtsGeneratedAspire.verified.go |
Captures generated runtime and union-check changes. |
tests/Aspire.Hosting.CodeGeneration.Go.Tests/AtsGoCodeGeneratorTests.cs |
Adds error, callback-binding, and token-forwarding regressions. |
src/Aspire.Hosting.CodeGeneration.Go/Resources/transport.go |
Updates error formatting, callback binding, and token forwarding. |
src/Aspire.Hosting.CodeGeneration.Go/AtsGoCodeGenerator.cs |
Emits union error checks and client-aware DTO callbacks. |
src/Aspire.Hosting.CodeGeneration.Go/AtsGoCodeGenerator.ApiProjection.cs |
Corrects the exported builder Err() documentation. |
Review effort: Balanced
…leReference in unions Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector4 / 101 PR test projects · 4 PR jobs, from 8 changed files. Selected PR test projects (4 / 101)
Selected PR jobs (4)
How these were chosen — grouped by what changed🔧 🔧 📦 affected project 🔧 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
Follow-up to #22057 (already on
main) addressing its review feedback:FormatErrornow finds joined errors behindfmt.Errorf("...: %w", ...)wrappers.callbackArg[*CancellationToken].Err()before sending the capability.CancellationTokenreuses the host-issued ID, so cancelling the host token cancels the nested operation.ErrAPI summary now describes the aggregated errors.Intentionally not done: capping
CancellationTokenRegistryID size. It requires an authenticated guest that already controls the connection.Added Go regression tests; snapshots updated accordingly.
Test:
.\dotnet.cmd test --project tests/Aspire.Hosting.CodeGeneration.Go.Tests/Aspire.Hosting.CodeGeneration.Go.Tests.csproj --no-launch-profile -- --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"— first run failed 4 (snapshot updates), rerun after accepting snapshots: 137 passed, 0 failed.Relates to #22057
Checklist