Fix PR channel version selection in Aspire CLI#16125
Fix PR channel version selection in Aspire CLI#16125sebastienros wants to merge 25 commits intomainfrom
Conversation
sebastienros
requested review from
JamesNK,
davidfowl and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16125Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16125" |
There was a problem hiding this comment.
Pull request overview
This PR fixes version drift when the Aspire CLI is running against PR hives / pr-* channels by preferring the currently installed CLI version during template and package selection, preventing mismatched stable vs PR-build dependencies.
Changes:
- Prefer the current CLI/SDK version when resolving templates/packages from
pr-*channels (and when PR hives are present foraspire add). - Add unit tests covering
aspire new,aspire init, andaspire addPR-channel/PR-hive selection behavior. - Extend a shared test
IPackagingServicehelper to allow test-controlled channel enumeration.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/TestServices/TestPackagingService.cs | Adds a callback hook to control GetChannelsAsync in tests. |
| tests/Aspire.Cli.Tests/Commands/NewCommandTests.cs | Adds coverage ensuring PR channel selects the current CLI version without prompting. |
| tests/Aspire.Cli.Tests/Commands/InitCommandTests.cs | Adds coverage ensuring PR channel selects the current CLI version without prompting. |
| tests/Aspire.Cli.Tests/Commands/AddCommandTests.cs | Adds coverage ensuring PR hives cause add to prefer the current CLI version. |
| src/Aspire.Cli/Utils/VersionHelper.cs | Introduces IsPrChannel helper for consistent PR-channel detection. |
| src/Aspire.Cli/Templating/DotNetTemplateFactory.cs | Prefers current CLI version when selecting templates from pr-* channels. |
| src/Aspire.Cli/Commands/NewCommand.cs | Prefers current CLI version when resolving template version from a pr-* channel. |
| src/Aspire.Cli/Commands/InitCommand.cs | Prefers current CLI version when selecting templates from pr-* channels. |
| src/Aspire.Cli/Commands/AddCommand.cs | Prefers current CLI version for integration packages when PR hives are present. |
|
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.
|
|
/deployment-test |
1 similar comment
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16125... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
/deployment-test |
|
🚀 Deployment tests starting on PR #16125... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
🚀 Deployment tests starting on PR #16125... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sebastienros
force-pushed
the
sebastienros/sebros-fix-cli-prerelease-install
branch
from
70dbd27 to
36327f3
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ix-cli-prerelease-install # Conflicts: # tests/Aspire.Deployment.EndToEnd.Tests/AcaCompactNamingUpgradeDeploymentTests.cs
…ix-cli-prerelease-install
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.
|
This reverts commit ab9af5c.
Reproduced the missing SDK source-mapping failure locally before reapplying this change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ix-cli-prerelease-install
|
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.
|
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Reviewed 13 changed files (982 additions, 20 deletions). Found 1 issue (resilience/bug category).
| using var projectDocument = _fallbackProjectParser.ParseProject(appHostProjectFile); | ||
| if (!projectDocument.RootElement.TryGetProperty("Properties", out var properties) || | ||
| !properties.TryGetProperty("AspireHostingSDKVersion", out var sdkVersionElement)) | ||
| { | ||
| return packageIds; | ||
| } | ||
|
|
||
| var sdkVersion = sdkVersionElement.GetString(); | ||
| if (!string.Equals(sdkVersion, selectedPackageVersion, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return packageIds; | ||
| } | ||
|
|
||
| packageIds.Add("Aspire.AppHost.Sdk"); |
There was a problem hiding this comment.
_fallbackProjectParser.ParseProject(appHostProjectFile) throws ProjectUpdaterException if the project file is malformed or unreadable. This propagates through the NuGet config creation block (lines 216–223) and hits the generic catch (Exception ex) in ExecuteAsync, failing the entire aspire add command with a misleading error about package addition failure.
The method already handles the "file doesn't exist" case gracefully (line 381) but not the "file exists but can't be parsed" case. Since determining whether to include Aspire.AppHost.Sdk in scoped mappings is a best-effort enhancement, a parsing failure should not prevent the package from being added.
Consider wrapping the ParseProject call in a try-catch and returning packageIds (just the selected package) on failure, consistent with the existing "file doesn't exist" fallback:
JsonDocument? projectDocument;
try
{
projectDocument = _fallbackProjectParser.ParseProject(appHostProjectFile);
}
catch (ProjectUpdaterException)
{
return packageIds;
}
using (projectDocument)
{
// existing property checks...
}
|
|
||
| private static HashSet<string> GetScopedPackageIds(string source, IEnumerable<string> packageIds) | ||
| { | ||
| var resolvedPackageIds = new HashSet<string>(packageIds, StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Add a NuGetPackageId comparer and comparison to known comparers file. It would be OrdinalIgnoreCase still, but centralizing values would help having consistent comparison be used.
Update code to use central value.
| .GroupBy(metadata => metadata.PackageId, StringComparer.OrdinalIgnoreCase) | ||
| .ToDictionary( | ||
| group => group.Key, | ||
| group => group.OrderByDescending(metadata => metadata.Version, SemVersion.PrecedenceComparer).First(), | ||
| StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
More places to use the comparer
| return []; | ||
| } | ||
| catch (InvalidDataException) | ||
| { | ||
| return []; | ||
| } | ||
| catch (System.Xml.XmlException) | ||
| { | ||
| return []; |
There was a problem hiding this comment.
I think there should be at least some debug level logging.
| private sealed class CallbackNuGetPackageCache( | ||
| Func<DirectoryInfo, bool, FileInfo?, CancellationToken, Task<IEnumerable<Aspire.Shared.NuGetPackageCli>>> getTemplatePackagesAsyncCallback) : INuGetPackageCache |
There was a problem hiding this comment.
Use a shared test type. Trying to avoid many copies.
|
Wouldn't we still want to be able to do |
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Reviewed the core fix — the version selection logic changes look correct and consistent across all command flows (add, new, init, template). Making channelName and hasPrHives required parameters on TryGetCurrentCliVersionMatch was the right call to prevent future callers from forgetting to pass them.
This should fix the stale package version mixing that causes the IContainerRuntime/AcrLoginService DI failure (#16229).
Note: @JamesNK has some open feedback items (ParseProject error handling, NuGet ID comparer centralization, debug logging, shared test type) that should be addressed before merge.
PythonFastApi, AppServicePython, and AcrPurgeTask deployment tests fail with 'Unable to resolve service for type IContainerRuntime while attempting to activate AcrLoginService' due to stable/dev package version mixing during aspire deploy. Tracked by #16229, fix incoming via PR #16125. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Wrap ParseProject in try-catch for ProjectUpdaterException in AddCommand.GetScopedPrHivePackageIds so a malformed project file does not crash the entire 'aspire add' command (best-effort check). - Add NuGetPackageId to the shared StringComparers/StringComparisons file and use it consistently in PackageChannel.cs for all NuGet package ID comparisons instead of inline StringComparer.OrdinalIgnoreCase. - Add debug-level logging to the catch blocks in PackageChannel.GetDependencyPackageIds so IO, invalid data, and XML parse errors are no longer silently swallowed. Thread an optional ILogger through PackageChannel and wire it from PackagingService via DI. - Extract the file-local CallbackNuGetPackageCache from InitCommandTests into a shared test service under TestServices/ to reduce duplication. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaced by WaitForAspireAddSuccessAsync during merge conflict resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Unify deployment E2E install helpers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Standardize deployment E2E CLI install Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix deployment E2E test failures - Bump Azure.Identity from 1.13.2 to 1.18.0 in FoundryHostedAgentDeploymentTests to fix NU1605 package downgrade error - Increase AcaManagedRedis test timeout from 30min to 40min and pipeline wait from 30min to 35min to accommodate slow Azure Redis provisioning - Increase AppServiceReact pipeline wait from 30min to 35min to accommodate slow Azure App Service provisioning Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix bundle layout for deployment E2E tests The deployment workflow only packaged the CLI binary and NuGet packages but not the managed AppHost server (aspire-managed). Tests that use InstallCurrentBuildAspireBundleAsync (TypeScript, Python, AppService, AcrPurge templates) require a valid bundle layout with managed/ and dcp/ directories for layout discovery to succeed. - Copy Aspire.Managed build output into CLI artifacts - Place managed server at ~/.aspire/managed/ during install - Create dcp/ directory stub for layout validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Disable Python deployment tests blocked by IContainerRuntime issue PythonFastApi, AppServicePython, and AcrPurgeTask deployment tests fail with 'Unable to resolve service for type IContainerRuntime while attempting to activate AcrLoginService' due to stable/dev package version mixing during aspire deploy. Tracked by #16229, fix incoming via PR #16125. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Disable Foundry test (quota), fix upgrade test backup/restore - Disable FoundryHostedAgentDeploymentTests pending Azure AI quota allocation (#16330) - Fix AcaCompactNamingUpgradeDeploymentTests: backup/restore now includes managed/ and dcp/ directories so the dev bundle layout survives the GA CLI install/restore cycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Disable AcaCompactNamingUpgradeDeploymentTests (#16332) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Mitch Denny <midenn@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The scoped NuGet config mapped Aspire* packages exclusively to the PR hive directory, which broke transitive RID-specific dependencies like Aspire.Hosting.Orchestration.linux-x64 that aren't included in the hive. The version selection fix (TryGetCurrentCliVersionMatch) already ensures the correct PR version is selected without needing the scoped config. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of scoping to individual package IDs (which missed transitive RID-specific deps like Aspire.Hosting.Orchestration.linux-x64), pass the original channel with its Aspire* wildcard mapping. This ensures all Aspire packages can resolve from the PR hive while still allowing fallback to NuGet.org via the * mapping. Also removes the GetScopedPrHivePackageIds method and CreateScopedChannelForPackages call which are no longer needed, and updates the test to verify the Aspire* wildcard is present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
dismissed
their stale review
Revoking my own approval since I've made a bunch of changes and want a second pair of eyes around intent.
|
🎬 CLI E2E Test Recordings — 72 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24672158800 |
Description
PR-built CLI installs can see both stable feed results and PR hive packages at the same time. That let
aspire add,aspire new, andaspire initdrift to a stable or latest version instead of the exact PR build version, which caused mismatches like stable AppHost package requests against prerelease-only PR hives.This change prefers the current CLI version whenever a
pr-*channel or PR hive is in play across package and template selection. It updates the shared .NET template selection path as well, adds focused unit coverage foradd,new, andinit, and reuses the shared test packaging helper instead of another file-local implementation.Fixes # N/A
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: