[release/13.2] Fix legacy settings migration path adjustment for appHostPath#15352
Merged
joperezr merged 9 commits intorelease/13.2from Mar 19, 2026
Merged
[release/13.2] Fix legacy settings migration path adjustment for appHostPath#15352joperezr merged 9 commits intorelease/13.2from
joperezr merged 9 commits intorelease/13.2from
Conversation
When migrating from .aspire/settings.json to aspire.config.json, the appHostPath was copied verbatim. But .aspire/settings.json stores paths relative to the .aspire/ directory (e.g., '../src/apphost.ts'), while aspire.config.json stores paths relative to its own directory (the project root). This caused the migrated path to resolve incorrectly. The fix re-bases the appHostPath during migration: resolves it against the legacy .aspire/ directory, then makes it relative to the config directory. For example, '../src/apphost.ts' becomes 'src/apphost.ts'. Includes unit tests for the path adjustment and an E2E test that verifies the full migration scenario with a TypeScript apphost in a subdirectory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15352Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15352" |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the legacy .aspire/settings.json → aspire.config.json migration so appHostPath is re-based from “relative to .aspire/” to “relative to the project root”, and adds tests to prevent regressions.
Changes:
- Re-base migrated
appHostPathduringAspireConfigFile.LoadOrCreate()legacy migration. - Add unit tests covering relative, root, already-
.aspire-local, saved-config, and absolute-path cases. - Add an end-to-end Docker test verifying the migration produces the corrected path in
aspire.config.json.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Aspire.Cli/Configuration/AspireConfigFile.cs | Adjusts legacy-migrated appHostPath to be relative to the config directory. |
| tests/Aspire.Cli.Tests/Configuration/AspireConfigFileTests.cs | Adds unit coverage for the re-basing behavior and persistence. |
| tests/Aspire.Cli.EndToEnd.Tests/LocalConfigMigrationTests.cs | Adds an E2E regression test ensuring migration works in a real CLI run flow. |
You can also share your feedback on Copilot code review. Take the survey.
The terminal buffer accumulates all output, so the earlier echo of legacy settings.json content (containing '../src/apphost.ts') would cause false matches when verifying the migrated aspire.config.json. Use 'grep' with the exact JSON key-value pattern to isolate the check, and rely on the host-side file verification as the primary assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
The transient CI rerun workflow requested reruns for the following jobs after analyzing the failed attempt.
|
The previous test moved apphost.ts to src/ but left .modules/ in the workspace root, breaking TS imports. aspire run couldn't start, so the migration never triggered and aspire.config.json was never created. Changes: - Keep apphost.ts at workspace root (project stays valid) - Test re-basing with '../apphost.ts' -> 'apphost.ts' instead (same code path as '../src/apphost.ts' -> 'src/apphost.ts') - Wait for 'Press CTRL+C' or 'Apphost failed' instead of generic 'ERR:' which could false-match shell prompt patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previous approach failed because: 1. aspire run error messages varied by failure mode 2. Terminal buffer pattern matching was fragile across exit codes Now polls the host-side filesystem via bind mount for aspire.config.json, which is created during apphost discovery before the actual run attempt. This works regardless of whether aspire run succeeds or fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
The transient CI rerun workflow requested reruns for the following jobs after analyzing the failed attempt.
|
- Normalize backslash separators to OS-native before Path operations,
then always output forward slashes (matching storage convention)
- Guard against empty/whitespace paths to avoid converting '' to '.'
- Use { Length: > 0 } pattern match instead of separate null check
- Rename misleading test to clarify .aspire/-relative semantics
- Add tests: backslash normalization, output always uses '/',
deeply nested paths, empty path preservation, null path handling
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous test used 'apphost.ts' without '../' prefix, which would mean the apphost lives inside .aspire/ — an impossible real-world scenario. Replace with a realistic .csproj-in-subdirectory case: '../MyApp.AppHost/MyApp.AppHost.csproj' -> 'MyApp.AppHost/MyApp.AppHost.csproj' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
I know you are still working on this but marking as servicing approved as it will be important for existing users. Let me know when it's ready for review and ready to go. |
Replace manual slash normalization + GetFullPath with the existing PathNormalizer.NormalizePathForCurrentPlatform() utility, which does the same thing in one call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JamesNK
reviewed
Mar 18, 2026
JamesNK
reviewed
Mar 18, 2026
Address review feedback: - Extract PathNormalizer.NormalizePathForStorage() for the common backslash-to-forward-slash normalization used when persisting paths. - Add tests for ./path, bare relative path, Unix rooted, and Windows rooted legacy migration scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid test paths that imply apphost lives under .aspire/ since that never happens in practice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 2 jobs were identified as retry-safe transient failures in the CI run attempt.
|
Contributor
|
🎬 CLI E2E Test Recordings — 53 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23272314953 |
mitchdenny
changed the title
JamesNK
approved these changes
Mar 19, 2026
joperezr
approved these changes
Mar 19, 2026
Copilot AI
pushed a commit
that referenced
this pull request
Mar 19, 2026
…ostPath (#15352) * Fix legacy settings migration path adjustment When migrating from .aspire/settings.json to aspire.config.json, the appHostPath was copied verbatim. But .aspire/settings.json stores paths relative to the .aspire/ directory (e.g., '../src/apphost.ts'), while aspire.config.json stores paths relative to its own directory (the project root). This caused the migrated path to resolve incorrectly. The fix re-bases the appHostPath during migration: resolves it against the legacy .aspire/ directory, then makes it relative to the config directory. For example, '../src/apphost.ts' becomes 'src/apphost.ts'. Includes unit tests for the path adjustment and an E2E test that verifies the full migration scenario with a TypeScript apphost in a subdirectory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix E2E test: use grep instead of terminal buffer check The terminal buffer accumulates all output, so the earlier echo of legacy settings.json content (containing '../src/apphost.ts') would cause false matches when verifying the migrated aspire.config.json. Use 'grep' with the exact JSON key-value pattern to isolate the check, and rely on the host-side file verification as the primary assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix E2E test: keep TS project intact, use robust patterns The previous test moved apphost.ts to src/ but left .modules/ in the workspace root, breaking TS imports. aspire run couldn't start, so the migration never triggered and aspire.config.json was never created. Changes: - Keep apphost.ts at workspace root (project stays valid) - Test re-basing with '../apphost.ts' -> 'apphost.ts' instead (same code path as '../src/apphost.ts' -> 'src/apphost.ts') - Wait for 'Press CTRL+C' or 'Apphost failed' instead of generic 'ERR:' which could false-match shell prompt patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix E2E test: poll host filesystem instead of parsing terminal Previous approach failed because: 1. aspire run error messages varied by failure mode 2. Terminal buffer pattern matching was fragile across exit codes Now polls the host-side filesystem via bind mount for aspire.config.json, which is created during apphost discovery before the actual run attempt. This works regardless of whether aspire run succeeds or fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR feedback: normalize separators, guard empty paths, add tests - Normalize backslash separators to OS-native before Path operations, then always output forward slashes (matching storage convention) - Guard against empty/whitespace paths to avoid converting '' to '.' - Use { Length: > 0 } pattern match instead of separate null check - Rename misleading test to clarify .aspire/-relative semantics - Add tests: backslash normalization, output always uses '/', deeply nested paths, empty path preservation, null path handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Replace unrealistic test with realistic subdirectory path scenario The previous test used 'apphost.ts' without '../' prefix, which would mean the apphost lives inside .aspire/ — an impossible real-world scenario. Replace with a realistic .csproj-in-subdirectory case: '../MyApp.AppHost/MyApp.AppHost.csproj' -> 'MyApp.AppHost/MyApp.AppHost.csproj' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use PathNormalizer for migration path re-basing Replace manual slash normalization + GetFullPath with the existing PathNormalizer.NormalizePathForCurrentPlatform() utility, which does the same thing in one call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add NormalizePathForStorage helper and additional path tests Address review feedback: - Extract PathNormalizer.NormalizePathForStorage() for the common backslash-to-forward-slash normalization used when persisting paths. - Add tests for ./path, bare relative path, Unix rooted, and Windows rooted legacy migration scenarios. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use realistic paths in migration tests Avoid test paths that imply apphost lives under .aspire/ since that never happens in practice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Mitch Denny <mitch@mitchdeny.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix legacy
.aspire/settings.json→aspire.config.jsonmigration producing incorrect relative paths for theappHostPath.Problem: When migrating from the legacy
.aspire/settings.jsonformat to the newaspire.config.json, theappHostPathwas copied verbatim byFromLegacy(). However, the legacy format stores paths relative to the.aspire/directory (e.g.,../src/apphost.ts), while the new format stores paths relative to the config file's own directory (the project root). This caused migrated paths to resolve incorrectly — for example,../src/apphost.ts(valid from.aspire/) becomes invalid from the project root because it resolves above the repo.Fix: After
FromLegacy()migration inLoadOrCreate(), the path is re-based: resolved against the legacy.aspire/directory, then made relative to the config directory. So../src/apphost.tscorrectly becomessrc/apphost.ts.Fixes # (issue)
Checklist