Repository navigation
Preserve ASPIRE010 and add ASPIRE013 for required CLI bundles - #21288
Conversation
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 -- 21288Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 21288" |
github-actions
Bot
added
the
needs-area-label
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟢 Approval recommended
The focused diagnostic changes match the intended contracts, include regression coverage, and have no unresolved blocking findings.
No open issues 🎉
What changed in this PR
Preserves Aspire diagnostic compatibility by restoring ASPIRE010’s warning contract and assigning ASPIRE013 to the existing C# CLI bundle requirement.
Changes:
- Restores suppressible ASPIRE010 for non-C# AppHosts and introduces mutually exclusive, unsuppressible ASPIRE013 for C# AppHosts.
- Expands regression coverage for bundle settings, language applicability, suppression, and emitted diagnostics.
- Updates diagnostic and migration documentation without changing ASPIRE012.
| File | Description |
|---|---|
tests/Aspire.Hosting.Sdk.Tests/AppHostSdkTargetsTests.cs |
Adds diagnostic and suppression test cases. |
src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.in.targets |
Separates optional-bundle warnings from required-bundle errors. |
docs/migrate-from-apphost-sdk.md |
References ASPIRE013 for the required bundle. |
docs/list-of-diagnostics.md |
Documents both diagnostic contracts and language applicability. |
Review effort: Balanced
|
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. |
Sébastien Ros (sebastienros)
left a comment
Sébastien Ros (sebastienros)
left a comment
There was a problem hiding this comment.
The C#/non-C# split looks correct: the two diagnostics are mutually exclusive, ASPIRE010 matches its previous wording and severity for non-C# AppHosts, and the C# requirement is unchanged. I found 4 minor issues and no blocking bugs: 1 diagnostic ID conflict, 1 help link and 2 test gaps.
This is a comment rather than an approval because auto-merge is on, and an approval could merge the PR before these are read.
One issue is outside the diff, so it can't be an inline comment: CliBundleDefaultResolvesBundleWithoutOptIn in tests/Aspire.Hosting.Tests/MSBuildTests.cs:443 still has Assert.DoesNotContain("ASPIRE010", output). That test builds a C# AppHost, which can no longer emit ASPIRE010, so the assertion can't fail. If the default regressed, the build would fail with ASPIRE013 and BuildProject's exit-code check would catch it, so the line can be removed.
Sébastien Ros (sebastienros)
left a comment
Sébastien Ros (sebastienros)
left a comment
There was a problem hiding this comment.
I found no new issues. The three open threads from my earlier review are all minor and can be handled after merge:
- The
ASPIRE013ID conflict is best fixed by renumbering in #21592 (and in draft #19880), so this PR can merge first. - The
aspire013aka.ms redirect and the aspire.dev article are documentation follow-ups that are already listed in the PR description. - Covering the default-off path for non-C# AppHosts is a test follow-up.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector45 / 101 PR test projects · 3 PR jobs, from 3 changed files. Selected PR test projects (45 / 101)
Selected PR jobs (3)
How these were chosen — grouped by what changed
🔧 show 43
📦 affected project 🧪 🧪 Job reasons
Selection computed for commit |
|
Pull request created: #2071
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#2071 targeting Adds ASPIRE013 diagnostic page, scopes ASPIRE010 to non-C# AppHosts, and updates overview, aspire-sdk, and ASPIRE009 pages. Signals: diagnostic_documentation_changed (docs/list-of-diagnostics.md), pr_body_has_cli_flag_mention, pr_body_has_user_facing_section. Note This draft PR needs human review before merging. |
## Description `AspireCliInvocationMode` only recognized `Dnx` and `DnxPinned`; every other value (for example `DnxPinnedd` or `DefinitelyInvalid`) silently fell through to `Path` resolution, so typos never surfaced. **Root cause:** `_ResolveAspireCliInvocation` passed the raw value to `ResolveAspireCliInvocation`, which treats anything other than `Dnx`/`DnxPinned` as `Path`. The only validation target (`_ValidateAspireCliDnxInvocationMode`) is conditioned on a DNX mode, so it was skipped for unrecognized values. **Fix:** `_ResolveAspireCliInvocation` emits a new suppressible `ASPIRE014` warning when the value is not `Path`, `Dnx` or `DnxPinned` (case-insensitive), and resolution continues in `Path` mode as before. I chose a warning instead of an error so that existing builds keep working while the mistake becomes visible. `ASPIRE013` is skipped because open PRs #21288 and #19880 both propose it. `docs/list-of-diagnostics.md` documents `ASPIRE014`. Fixes #19714 ### Before (main) Playground `DotnetProject.AppHost` with `-p:AspireUseCliBundle=true -p:AspireCliInvocationMode=DefinitelyInvalid -t:_ResolveAspireCliInvocation -v:diag`: ```text Task Parameter:AspireCliInvocationMode=DefinitelyInvalid Aspire CLI invocation selection: Dnx (aspire: , dnx: /root/aspire/.dotnet/dnx) <- Path-mode fallback (no aspire on PATH) 0 Error(s) 0 Warning(s) ``` A full `dotnet build` of the AppHost with the same value also reported `0 Warning(s)`. ### After ```text Aspire.Hosting.AppHost.in.targets(221,5): warning ASPIRE014: DotnetProject.AppHost is configured with AspireCliInvocationMode=DefinitelyInvalid, which is not a supported value. Supported values are Path, Dnx, and DnxPinned. The default Path invocation mode will be used. Aspire CLI invocation selection: Dnx (aspire: , dnx: /root/aspire/.dotnet/dnx) 0 Error(s) 1 Warning(s) ``` Valid values (`Path`, `DnxPinned`) still resolve to `Aspire` and `Dnx` without a warning. ### End-to-end with Aspire I ran `dotnet run` on `playground/DotnetProject/DotnetProject.AppHost` with `AspireCliInvocationMode=DefinitelyInvalid` and the Aspire CLI built from this branch (`17.0.0-dev`) on `PATH`. The warning appears during the build, and the AppHost then launches through `aspire run` (Path mode): ```text Building... ...Aspire.Hosting.AppHost.in.targets(221,5): warning ASPIRE014: DotnetProject.AppHost is configured with AspireCliInvocationMode=DefinitelyInvalid, which is not a supported value. Supported values are Path, Dnx, and DnxPinned. The default Path invocation mode will be used. Connecting to AppHost... Starting dashboard... AppHost: DotnetProject.AppHost.csproj Press CTRL+C to stop the AppHost and exit. ``` (The dashboard was skipped locally with `SkipDashboardProjectReference=true`.) ### Tests New tests in `tests/Aspire.Hosting.Sdk.Tests/AppHostSdkTargetsTests.cs`: - `ComputeRunArgumentsWarnsAndUsesPathModeWhenInvocationModeIsUnsupported` (`DefinitelyInvalid`, `DnxPinnedd`): asserts that `ASPIRE014` is emitted and that the fake `aspire` from `PATH` is selected. - `ComputeRunArgumentsDoesNotWarnWhenInvocationModeIsSupported` (unset, `Path`, `pAtH`, `Dnx`, `DnxPinned`). - `UnsupportedInvocationModeWarningCanBeSuppressed` (`NoWarn=ASPIRE014`). Without the fix, the two unsupported-value cases fail (`total: 8, failed: 2`). With the fix, the full `Aspire.Hosting.Sdk.Tests` project passes: 98 total, 94 passed, 0 failed, 4 skipped (Windows-only). ### Playground No playground file changed. In-repo playground AppHosts don't import `Aspire.Hosting.AppHost.props`, so `AspireUseCliBundle` isn't enabled by default and the packaged tasks assembly isn't present. Setting `AspireCliInvocationMode` in a playground project would therefore have no effect in a normal repo build. The playground proof above runs the existing `DotnetProject.AppHost` and passes those properties on the command line. ## Checklist - Is this feature complete? - [x] Yes. Ready to ship. - [ ] No. Follow-up changes expected. - Are you including unit tests for the changes and scenario tests if relevant? - [x] Yes - [ ] No - Did you add public API? - [ ] Yes - [x] No - Does the change make any security assumptions or guarantees? - [ ] Yes - [x] No **🤖 Posted by AspireAgent (AI Agent)** Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Diagnostic IDs must keep their meaning across releases. #20867 changed ASPIRE010 from a suppressible warning about optional CLI bundle features into an unsuppressible error requiring the bundle. Existing suppression settings and the published diagnostic reference therefore describe a different contract.
Restore ASPIRE010's original warning text, severity, and help link for non-C# AppHosts, where opting out remains supported. Introduce ASPIRE013 for the required-bundle error in C# AppHosts, without changing the C# bundle requirement or ASPIRE012 SDK-obsolescence warning. The two diagnostics are mutually exclusive.
Update the product diagnostic list and migration guide, and cover enabled/disabled bundles, C#/non-C# applicability, both warning IDs in NoWarn, and the complete set of emitted Aspire diagnostics.
Related documentation discussion: microsoft/aspire.dev#1880. That documentation PR should preserve the ASPIRE010 warning reference and document ASPIRE013 separately. Publishing the ASPIRE013 article and configuring https://aka.ms/aspire/diagnostics/aspire013 are coordinated documentation follow-ups; this product PR does not publish the site or configure the redirect.
User-facing usage
For a C# AppHost, this continues to fail the build, now with
error ASPIRE013rather than a repurposed ASPIRE010:For a non-C# AppHost, opting out emits the original
warning ASPIRE010, which remains suppressible with<NoWarn>$(NoWarn);ASPIRE010</NoWarn>. Adding either ASPIRE010 or ASPIRE013 to NoWarn does not bypass the C# requirement.Validation
.\restore.cmd— passed.dotnet build tests\Aspire.Hosting.Sdk.Tests\Aspire.Hosting.Sdk.Tests.csproj --no-restore --verbosity quiet— passed, with four NU1902/NU1903 audit warnings for System.Security.Cryptography.Pkcs 10.0.12. No dependency versions are changed by this PR.git diff --check— passed.dotnet testinvocation below built the project but timed out in a child MSBuild invocation and was stopped. Retrying the diagnostic matrix with NuGet audit disabled also timed out and was stopped. Running the same selected tests with the built native MTP executable succeeded as recorded above.Full solution and cross-platform validation were not run locally.
Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?