Add Aspire integration analyzer#1370
Conversation
Enable the Aspire.Hosting.Integration.Analyzers package for integration projects and clean up exported hosting APIs so the analyzer validates the polyglot surface. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.sh | bash -s -- 1370Or
iex "& { $(irm https://raw.githubusercontent.com/CommunityToolkit/Aspire/main/eng/scripts/dogfood-pr.ps1) } 1370" |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the codebase from using [AspireExport("name", Description = "...")] attribute syntax to a new pattern using [AspireExport] (or [AspireExport("explicitName")]) combined with <ats-summary> XML documentation tags. It also refactors a few files that used the C# 14 extension(...) block syntax back to traditional static extension methods, and adds the Aspire.Hosting.Integration.Analyzers package reference.
Changes:
- Replaces
Description = "..."arguments on[AspireExport]with<ats-summary>doc comments and drops redundant export name arguments when they match the method name. - Converts C# extension block syntax to traditional static extension methods in Flyway and Postgres extension files.
- Adds the new
Aspire.Hosting.Integration.Analyzerspackage source, version reference, and project-widePackageReference.
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Directory.Build.props, nuget.config, Directory.Packages.props |
Wire up the new Aspire.Hosting.Integration.Analyzers package globally. |
*/...BuilderExtensions.cs (many) |
Replace [AspireExport("...", Description=...)] with [AspireExport] + <ats-summary> doc tags; preserve explicit export names where they differ from method names; some add RunSyncOnBackgroundThread = true. |
Flyway/*.cs, PostgreSQL.Extensions/PostgresDatabaseResourceBuilderExtensions.cs |
Refactor extension(builder) { ... } blocks into classic this-parameter static extension methods. |
Perl/*.cs, Ollama/OllamaUtilities.cs, DuckDB/... |
Add [AspireExport]/[AspireExportIgnore] annotations and wrap files with #pragma warning disable ASPIREATS001. |
Comments suppressed due to low confidence (6)
src/CommunityToolkit.Aspire.Hosting.OpenTelemetryCollector/OpenTelemetryCollectorRoutingExtensions.cs:1
- The explicit export name
"withOpenTelemetryCollectorOpenTelemetryCollectorRouting"contains a duplicatedOpenTelemetryCollectorsegment. The previous export name was"withOpenTelemetryCollectorRouting", and renaming an export is a breaking change for polyglot consumers. This should either be reverted to"withOpenTelemetryCollectorRouting"or, since the method isWithOpenTelemetryCollectorRoutingand the export name would be auto-derived, simplified to[AspireExport]to match the pattern used elsewhere in this PR.
src/CommunityToolkit.Aspire.Hosting.SurrealDb/SurrealDbBuilderExtensions.cs:1 - Specifying both the export name
"withSurrealDbOtlpExporter"andMethodName = "withSurrealDbOtlpExporter"is redundant — whenMethodNameequals the export name, only one needs to be provided (consistent with the rest of the refactor where matching names are dropped). Consider simplifying to[AspireExport(MethodName = "withSurrealDbOtlpExporter")]or just[AspireExport("withSurrealDbOtlpExporter")]depending on the intended behavior.
src/CommunityToolkit.Aspire.Hosting.Sqlite/SqliteResourceBuilderExtensions.cs:1 - The export name and
MethodNameare identical ("withSqliteWeb"), which is redundant. To stay consistent with the simplification pattern applied elsewhere in this PR, drop one of the two arguments.
src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.cs:1 - Disabling the experimental warning at file scope (rather than scoping it more narrowly around the
[AspireExport]usages, or suppressing it in the project file) hides the experimental status from any future contributor adding code to this file. Consider either suppressingASPIREATS001in a single<NoWarn>entry in the csproj for clarity, or limiting the#pragmaregion to just the attribute usages.
src/CommunityToolkit.Aspire.Hosting.Perl/PerlAppResourceBuilderExtensions.cs:1 - Unlike the rest of this PR, the newly added
[AspireExport]attributes in this file (andPerlAppResourceBuilderExtensions.PackageManager.cs) have no accompanying<ats-summary>doc tag, so the exported polyglot surface will be missing the description that other integrations provide. Please add<ats-summary>comments for each newly exported method (AddPerlScript,AddPerlApi,AddPerlModule,AddPerlExecutable,WithPerlbrew,WithPerlbrewEnvironment,WithLocalLib,WithPerlCertificateTrust,WithCpanMinus,WithCarton,WithPackage,WithProjectDependencies).
src/CommunityToolkit.Aspire.Hosting.Rust/RustAppHostingExtension.cs:1 - The previous attribute had
Description = "Adds a Rust application to the application model"forAddBaconApp, which was already a copy of theAddRustAppdescription and didn't describe the bacon-specific behavior. Now that the description is gone with no<ats-summary>replacement, the exported surface has no description at all. Consider adding an accurate<ats-summary>such as "Adds a Rust application started with the bacon watcher" (and an equivalent one forAddRustApp).
Keep Flyway and PostgreSQL Flyway APIs using C# extension blocks and suppress only the analyzer diagnostic that currently treats exported extension-block members as non-static. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep ats-summary elements only when summaries contain XML tags and move those ATS summaries inside the summary block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move remaining ats-summary elements outside summary blocks while keeping them adjacent to the documented member. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aaronpowell
left a comment
There was a problem hiding this comment.
Some comments, and the branch should probably target the 13.4 upgrade branch so we keep it all in sync.
|
|
||
| internal static class OllamaUtilities | ||
| { | ||
| [AspireExportIgnore(Reason = "Internal helper returns a tuple and accepts CancellationToken; it is not part of the polyglot surface.")] |
There was a problem hiding this comment.
Won't the fact this is on an internal class mean that it won't be exported?
There was a problem hiding this comment.
The analyzer detects it as an exported extension, it's likely a bug in the analyzer but until it's fixed we need to ignore the method.
| /// <param name="collectorBuilder">The OpenTelemetry Collector resource builder.</param> | ||
| /// <returns>A reference to the resource builder.</returns> | ||
| [AspireExport("withOpenTelemetryCollectorRouting", Description = "Routes telemetry for a resource through the specified OpenTelemetry Collector")] | ||
| [AspireExport("withOpenTelemetryCollectorOpenTelemetryCollectorRouting")] |
There was a problem hiding this comment.
This exported method name looks pretty clunky, does it need OpenTelemetryCollector in there twice?
| /// <summary> | ||
| /// Adds a Zitadel container resource to the <see cref="IDistributedApplicationBuilder"/>. | ||
| /// </summary> | ||
| /// <ats-summary>Adds a Zitadel container resource</ats-summary> |
There was a problem hiding this comment.
I think this is the only usage of ats-summary. I assume that's to make something appear for the polyglot apphosts, but why is it only on this method?
There was a problem hiding this comment.
The idea is that if the standard <summary> contains special C# tags (<see cref) or type names (generics) then we need to provide one for polyglot. There is the equivalent for params and return values.
| /// <summary> | ||
| /// Configures the external domain for the Zitadel resource. This overrides the default domain set in <see cref="AddZitadel"/>. | ||
| /// </summary> | ||
| /// <ats-summary>Configures the external domain for the Zitadel resource</ats-summary> |
There was a problem hiding this comment.
Ok, there was a second usage 😅
Use the cleaner OpenTelemetry Collector routing export name and suppress the conservative collision diagnostic locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Suppress ASPIREATS001 in the Perl project file instead of using file-scoped pragmas around exported members. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Aaron Powell <me@aaron-powell.com>
This enables the Aspire hosting integration analyzer so CommunityToolkit integrations are continuously checked for polyglot-compatible exported APIs.
The change wires
Aspire.Hosting.Integration.Analyzersfrom the dotnet9 feed into integration projects as a private analyzer dependency, then updates exported hosting APIs to satisfy the analyzer. The cleanup removes redundant export IDs, keepsats-summarydocumentation only where summaries contain C# XML tags, marks synchronous callback exports for background-thread execution, and explicitly exports or ignores members that affect the generated polyglot surface.A few integrations needed non-obvious handling: C# extension-block syntax is preserved for Flyway and PostgreSQL Flyway helpers, with a narrow suppression for the analyzer diagnostic that currently treats exported extension-block members as non-static.