Gate AppHost dotnet run hook on Aspire CLI version#18360
Gate AppHost dotnet run hook on Aspire CLI version#18360DamianEdwards wants to merge 15 commits into
Conversation
Add version checking to prevent delegation to 'aspire run' when the resolved Aspire CLI is older than 13.5.0, which would cause recursion with file-based AppHosts. Implements minimum CLI version gate as requested in #18359. Changes: - Created GetAspireCliVersion MSBuild task to detect CLI version - Modified AppHost SDK targets to check CLI version before delegating to 'aspire run' - Added comprehensive tests covering versions < 13.5.0, >= 13.5.0, and unparseable This prevents version-skew recursion when the SDK expects delegate behavior that older CLI versions don't support. Co-authored-by: Copilot <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 -- 18360Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18360" |
This comment has been minimized.
This comment has been minimized.
|
@davidfowl super early attempt, haven't even looked at it yet |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #18359, where the AppHost SDK dotnet run hook can recurse with an older Aspire CLI on PATH. It introduces a new MSBuild task (GetAspireCliVersion) that probes aspire --version, and a new _AspireCheckCliVersion target that gates the dotnet run → aspire run delegation on the resolved CLI being at least 13.5.0, falling back to normal dotnet run when the CLI is older or its version can't be determined. New SDK target tests cover below-minimum, at/above-minimum, and unparseable-version cases.
Changes:
- New
GetAspireCliVersionMSBuild task that runsaspire --versionand parses the semver (net472 + net8.0 paths, 5s timeout). - New
_AspireCheckCliVersiontarget plus a_AspireCliVersionSupportsRunHookgate on_AspireComputeRunArguments. - New theory/fact tests and fake-CLI helpers; wires the built
Aspire.Hosting.Tasks.dllinto the test AppHost via_AspireTasksAssembly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
src/Aspire.Hosting.Tasks/GetAspireCliVersion.cs |
New task to resolve the CLI version; timeout branch leaks the process and mishandles the cancellation exception. |
src/Aspire.Hosting.AppHost/build/Aspire.Hosting.AppHost.in.targets |
Adds the version gate; below-minimum versions are not suppressed, the gate is skipped for PATH-resolved CLIs, a malformed property ref in a message, and a behavior change that breaks an existing test. |
tests/Aspire.Hosting.Sdk.Tests/AppHostSdkTargetsTests.cs |
New gate tests; hardcodes Release, uses assertions that can't detect suppression failures, and has grammatical test-name errors. |
Key concerns: the gate does not actually suppress delegation for a parseable below-minimum version (e.g. 13.4.0, the issue's own scenario) because _AspireCliVersionSupportsRunHook keeps its default true; the gate is also skipped entirely for the common PATH-resolved CLI case; and the new negative tests assert against the PATH-fallback command so they pass regardless. The empty-version handling additionally changes behavior asserted by the pre-existing ComputeRunArgumentsUsesConfiguredAspireCliPathWhenCliBundleIsEnabled test.
Ensure the run-hook version check runs before deciding whether to rewrite dotnet run arguments, including when the Aspire CLI is resolved from PATH. Tighten the SDK tests so unsupported, unavailable, or unparseable CLI versions assert normal dotnet run behavior instead of only checking inequality with the default aspire command. Also make the Hosting.Sdk test project build Aspire.Hosting.Tasks so CI can load the new MSBuild task from a clean artifact directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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. |
Skip the AppHost run-hook CLI version probe when the MSBuild task assembly is not available. This preserves normal dotnet run behavior for source-target imports in clean CI jobs instead of failing target evaluation before the package-layout tools folder exists. Add a regression test for the missing task assembly case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Start reading stdout and stderr before waiting for the Aspire CLI version probe to exit so a noisy CLI cannot block on a full pipe and be misdetected as unsupported. Add regression coverage for a supported CLI that writes a large stderr payload before printing its version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
When the Aspire CLI version probe times out, wait briefly for the killed process to exit and observe the pending stdout/stderr drain tasks so pipe cleanup does not leave unobserved task exceptions behind. Add regression coverage for a hanging version command falling back to normal dotnet run behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Kill timed-out Aspire CLI version probes as a process tree on modern target frameworks so shell-launched child processes do not keep redirected pipe handles open. Relax the timeout regression assertion to avoid flakiness on slow CI agents while still proving the probe falls back promptly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When AspireCliPath points to a Windows command script, run the version probe through cmd.exe so .cmd and .bat shims are handled consistently with PATH resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Update the version probe comment and fake Aspire CLI scripts to reflect that aspire --version emits a bare informational version. The parser still extracts the numeric version component so prerelease outputs satisfy the 13.5 floor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Strip any prerelease suffix from aspire --version output before validating with System.Version. Add coverage that prefixed text is treated as unparseable and falls back to normal dotnet run behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the version parsing comment generic to the task rather than tying it to the current run-hook minimum version. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use RuntimeInformation.IsOSPlatform instead of a directory separator proxy in the CLI version probe task. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Use the test target framework when exposing the built Aspire.Hosting.Tasks assembly path to Hosting.Sdk tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Resolve the built Aspire.Hosting.Tasks assembly path using the task framework selected for the current MSBuild runtime instead of assuming it matches the test target framework. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Disable cmd AutoRun for PATH-based Windows probes and bound stdout/stderr drain after a successful process exit so inherited pipe handles cannot hang the MSBuild task. Add regression coverage for a version probe whose parent exits while a child process keeps redirected output open. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. Runs the full test matrix + all jobs (ALL) — run-all fallback: '.gitignore' is neither Layer-1-owned nor matched by a Layer 2 rule Selection computed for commit |
Strip build metadata as well as prerelease suffixes before parsing aspire --version output, and add coverage for stable informational versions with metadata. Remove wall-clock assertions from timeout fallback tests so they do not depend on full MSBuild invocation timing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
dotnet runonly delegates toaspire runwhen the resolved Aspire CLI is at least 13.5dotnet runbehavior when the CLI is older, unavailable, or has an unparseable versionFixes #18359
Testing
dotnet test --project tests/Aspire.Hosting.Sdk.Tests/Aspire.Hosting.Sdk.Tests.csproj --no-launch-profile -- --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"