Avoid AppHost discovery when config path is valid#16653
Avoid AppHost discovery when config path is valid#16653adamint merged 3 commits intomicrosoft:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16653Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16653" |
e56c1c7 to
c09e5a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Aspire CLI AppHost resolution to short-circuit recursive AppHost discovery when aspire.config.json contains a valid, validated appHost.path, addressing expensive filesystem scans for TypeScript/Python AppHosts. It also tightens configuration policy around AppHost path keys (disallowing global usage and de-emphasizing the legacy appHostPath key) and adjusts schemas/tests accordingly.
Changes:
- Validate and use configured
appHost.pathdirectly for normal CLI resolution to avoid costly discovery scans; fall back to discovery when the configured path is stale/invalid. - Add configuration policy + CLI behavior to prevent setting AppHost paths globally and to discourage the legacy
appHostPathkey, including warnings and updated schemas. - Add/extend unit tests for the updated resolution behavior and config/schema policy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Projects/ProjectLocatorTests.cs | Adds test coverage for using validated configured AppHost paths without scanning and for candidate-listing behavior. |
| tests/Aspire.Cli.Tests/ProgramTests.cs | Adds tests for startup warning behavior when global settings contain AppHost path keys. |
| tests/Aspire.Cli.Tests/Configuration/AppHostPathConfigurationPolicyTests.cs | Adds tests for the new AppHost path configuration policy helpers. |
| tests/Aspire.Cli.Tests/Commands/SettingsSchemaBuilderTests.cs | Ensures generated schemas no longer advertise the legacy appHostPath key while keeping local appHost.path. |
| tests/Aspire.Cli.Tests/Commands/ConfigCommandTests.cs | Verifies aspire config set behavior for local AppHost path keys and rejects global/legacy AppHost path keys. |
| src/Aspire.Cli/Utils/ConfigurationHelper.cs | Adds helpers for legacy settings root detection and a safe settings loader for warning/policy checks. |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.zh-Hant.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.zh-Hans.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.tr.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.ru.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.pt-BR.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.pl.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.ko.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.ja.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.it.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.fr.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.es.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.de.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/xlf/ErrorStrings.cs.xlf | Adds new error string entries for global/legacy AppHost path restrictions (localization stub). |
| src/Aspire.Cli/Resources/ErrorStrings.resx | Adds new error messages for blocking/ignoring global AppHost path configuration and legacy key usage. |
| src/Aspire.Cli/Resources/ErrorStrings.Designer.cs | Regenerates strongly-typed accessors for the new error strings. |
| src/Aspire.Cli/Projects/ProjectLocator.cs | Implements validated configured AppHost selection and updated candidate listing selection behavior. |
| src/Aspire.Cli/Program.cs | Adds startup-time warning when global settings contain AppHost path keys. |
| src/Aspire.Cli/Configuration/HiddenFromConfigurationSchemaAttribute.cs | Introduces an attribute to suppress properties from generated configuration schemas. |
| src/Aspire.Cli/Configuration/ConfigurationService.cs | Ensures local appHost.path settings land in aspire.config.json (migrating from legacy settings when needed). |
| src/Aspire.Cli/Configuration/AspireJsonConfiguration.cs | Marks legacy appHostPath as supported-but-hidden from schema generation. |
| src/Aspire.Cli/Configuration/AppHostPathConfigurationPolicy.cs | Adds centralized policy helpers for identifying AppHost path keys and finding them in configuration. |
| src/Aspire.Cli/Commands/SettingsSchemaBuilder.cs | Skips schema generation for properties marked as hidden. |
| src/Aspire.Cli/Commands/ConfigCommand.cs | Blocks setting legacy/global AppHost path keys via aspire config set with targeted errors. |
| extension/schemas/aspire-settings.schema.json | Removes appHostPath from the legacy local settings schema. |
| .gitignore | Ignores .worktrees/ directory. |
Validate configured AppHost paths before using them and skip recursive discovery for normal CLI resolution when the configured path is already a valid AppHost. Preserve candidate listing behavior so extension commands can still enumerate all AppHost candidates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c09e5a1 to
31ddae5
Compare
|
Addressed all 3 Copilot review comments in commit
|
adamint
left a comment
There was a problem hiding this comment.
One drive-by finding from a self-review pass. Caught one potentially-unintentional behavior change in this PR.
Reverts an unintended verbosity change so users with a stale legacy .aspire/settings.json don't see a 'AppHost was specified but doesn't exist' warning on every aspire run when the CLI is going to fall back to discovery anyway. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/backport to release/13.3 |
|
Started backporting to |
- Add Throw-mode variant of UseOrFindAppHostProjectFileUsesValidSettingsWithoutScanning to prove recursive discovery is also skipped for non-interactive/JSON callers. - Add fast-path test with a configured guest AppHost (apphost.ts) and a throwing language discovery to cover the production guest-AppHost scenario from the issue. - Add a parameterized fallback test that verifies an existing-but-invalid configured AppHost (IsUnsupported / IsPossiblyUnbuildable) falls back to discovery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
/backport to release/13.3 |
|
Started backporting to |
Description
Updates AppHost resolution so a valid AppHost path from
aspire.config.jsonis validated and used directly for normal CLI resolution instead of continuing into recursive AppHost discovery. This avoids expensive scanning when TypeScript or Python AppHosts already have a configured path.The change also preserves candidate-listing behavior for callers that intentionally request all AppHost candidates, while still selecting the configured AppHost when it is among the discovered candidates. Stale or invalid configured paths now fall back to discovery instead of being accepted blindly.
Validation:
dotnet test --project tests/Aspire.Cli.Tests/Aspire.Cli.Tests.csproj --no-launch-profile -- --filter-method "*.UseOrFindAppHostProjectFileFallsBackWhenSettingsFileSpecifiesExistingNonAppHost" --filter-method "*.UseOrFindAppHostProjectFileScansWhenCandidateListingModeHasValidSettings" --filter-method "*.UseOrFindAppHostProjectFileUsesValidSettingsWithoutScanning" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"dotnet test --project tests/Aspire.Cli.Tests/Aspire.Cli.Tests.csproj --no-launch-profile -- --filter-class "*.ProjectLocatorTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"dotnet test --project tests/Aspire.Cli.Tests/Aspire.Cli.Tests.csproj --no-launch-profile -- --filter-class "*.RunCommandTests" --filter-class "*.ExtensionInternalCommandTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"~/tmp/Issue16595ConfiguredAppHostconfirmed the configuredapphost.tsis used without scanning.Fixes #16595
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: