-
Notifications
You must be signed in to change notification settings - Fork 891
Fix PR channel version selection in Aspire CLI #16125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
0a22ef9
39aff7d
61a46d4
f31eccc
742357b
b837e75
c46d315
15ac30a
36327f3
596c1ca
360f280
ffd8112
dfe1b33
0bd5fbb
ab9af5c
80dc3a2
faa7933
7a60155
5445cda
ce9b06e
3730e7d
d06cc06
e5cf124
d9b265a
72f579a
ffd58fe
d80bb9e
8572286
6d5e323
3b301d3
ff15330
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ internal sealed class AddCommand : BaseCommand | |
| private readonly IDotNetSdkInstaller _sdkInstaller; | ||
| private readonly ICliHostEnvironment _hostEnvironment; | ||
| private readonly IAppHostProjectFactory _projectFactory; | ||
| private readonly FallbackProjectParser _fallbackProjectParser; | ||
|
|
||
| private static readonly Argument<string> s_integrationArgument = new("integration") | ||
| { | ||
|
|
@@ -44,7 +45,7 @@ internal sealed class AddCommand : BaseCommand | |
| Description = AddCommandStrings.SourceArgumentDescription | ||
| }; | ||
|
|
||
| public AddCommand(IPackagingService packagingService, IInteractionService interactionService, IProjectLocator projectLocator, IAddCommandPrompter prompter, AspireCliTelemetry telemetry, IDotNetSdkInstaller sdkInstaller, IFeatures features, ICliUpdateNotifier updateNotifier, CliExecutionContext executionContext, ICliHostEnvironment hostEnvironment, IAppHostProjectFactory projectFactory) | ||
| public AddCommand(IPackagingService packagingService, IInteractionService interactionService, IProjectLocator projectLocator, IAddCommandPrompter prompter, AspireCliTelemetry telemetry, IDotNetSdkInstaller sdkInstaller, IFeatures features, ICliUpdateNotifier updateNotifier, CliExecutionContext executionContext, ICliHostEnvironment hostEnvironment, IAppHostProjectFactory projectFactory, FallbackProjectParser fallbackProjectParser) | ||
| : base("add", AddCommandStrings.Description, features, updateNotifier, executionContext, interactionService, telemetry) | ||
| { | ||
| _packagingService = packagingService; | ||
|
|
@@ -53,6 +54,7 @@ public AddCommand(IPackagingService packagingService, IInteractionService intera | |
| _sdkInstaller = sdkInstaller; | ||
| _hostEnvironment = hostEnvironment; | ||
| _projectFactory = projectFactory; | ||
| _fallbackProjectParser = fallbackProjectParser; | ||
|
|
||
| Arguments.Add(s_integrationArgument); | ||
| Options.Add(s_appHostOption); | ||
|
|
@@ -208,6 +210,19 @@ await Parallel.ForEachAsync(channels, cancellationToken, async (channel, ct) => | |
| }; | ||
|
|
||
| // Add the package using the appropriate project handler | ||
| if (string.IsNullOrEmpty(source) && VersionHelper.IsPrChannel(selectedNuGetPackage.Channel.Name)) | ||
| { | ||
| var nugetConfigPrompter = new NuGetConfigPrompter(InteractionService); | ||
| var scopedPackageIds = GetScopedPrHivePackageIds( | ||
| effectiveAppHostProjectFile, | ||
| selectedNuGetPackage.Package.Id, | ||
| selectedNuGetPackage.Package.Version); | ||
| await nugetConfigPrompter.CreateOrUpdateWithoutPromptAsync( | ||
| effectiveAppHostProjectFile.Directory!, | ||
| selectedNuGetPackage.Channel.CreateScopedChannelForPackages(scopedPackageIds), | ||
| cancellationToken); | ||
| } | ||
|
|
||
| context = new AddPackageContext | ||
| { | ||
| AppHostFile = effectiveAppHostProjectFile, | ||
|
|
@@ -294,7 +309,7 @@ await Parallel.ForEachAsync(channels, cancellationToken, async (channel, ct) => | |
| _ => throw new InvalidOperationException(AddCommandStrings.UnexpectedNumberOfPackagesFound) | ||
| }; | ||
|
|
||
| var packageVersions = possiblePackages.Where(p => p.Package.Id == selectedPackage.Package.Id); | ||
| var packageVersions = possiblePackages.Where(p => p.Package.Id == selectedPackage.Package.Id).ToArray(); | ||
|
|
||
| // If any of the package versions are an exact match for the preferred version | ||
| // then we can skip the version prompt and just use that version. | ||
|
|
@@ -304,6 +319,22 @@ await Parallel.ForEachAsync(channels, cancellationToken, async (channel, ct) => | |
| return preferredVersionPackage; | ||
| } | ||
|
|
||
| // When PR hives are present, prefer the package that exactly matches the installed | ||
| // CLI/SDK version so template- and add-generated projects stay on the same build. | ||
| var prChannelPackageVersions = packageVersions | ||
| .Where(p => VersionHelper.IsPrChannel(p.Channel.Name)) | ||
| .ToArray(); | ||
|
|
||
| if (VersionHelper.TryGetCurrentCliVersionMatch( | ||
| prChannelPackageVersions, | ||
| p => p.Package.Version, | ||
| out var cliVersionPackage, | ||
| channelName: null, | ||
| hasPrHives: ExecutionContext.GetPrHiveCount() > 0)) | ||
| { | ||
| return cliVersionPackage; | ||
| } | ||
|
|
||
| // In non-interactive mode, prefer the implicit/default channel first to keep | ||
| // package selection aligned with the project's configured feeds. Then select | ||
| // the latest version within the chosen channel. | ||
|
|
@@ -339,6 +370,35 @@ internal static (string FriendlyName, NuGetPackage Package, PackageChannel Chann | |
|
|
||
| return (friendlyName, packageWithChannel.Package, packageWithChannel.Channel); | ||
| } | ||
|
|
||
| private IReadOnlyCollection<string> GetScopedPrHivePackageIds(FileInfo appHostProjectFile, string selectedPackageId, string selectedPackageVersion) | ||
| { | ||
| var packageIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| selectedPackageId | ||
| }; | ||
|
|
||
| if (!appHostProjectFile.Exists) | ||
| { | ||
| return packageIds; | ||
| } | ||
|
|
||
| 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"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 Consider wrapping the JsonDocument? projectDocument;
try
{
projectDocument = _fallbackProjectParser.ParseProject(appHostProjectFile);
}
catch (ProjectUpdaterException)
{
return packageIds;
}
using (projectDocument)
{
// existing property checks...
} |
||
| return packageIds; | ||
| } | ||
| } | ||
|
|
||
| internal interface IAddCommandPrompter | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.IO.Compression; | ||
| using System.Xml.Linq; | ||
| using Aspire.Cli.NuGet; | ||
| using Aspire.Cli.Resources; | ||
| using Aspire.Cli.Utils; | ||
| using Semver; | ||
| using NuGetPackage = Aspire.Shared.NuGetPackageCli; | ||
|
|
||
|
|
@@ -193,6 +196,172 @@ public async Task<IEnumerable<NuGetPackage>> GetPackagesAsync(string packageId, | |
| return filteredPackages; | ||
| } | ||
|
|
||
| public PackageChannel CreateScopedChannelForPackage(string packageId) | ||
| { | ||
| return CreateScopedChannelForPackages([packageId]); | ||
| } | ||
|
|
||
| public PackageChannel CreateScopedChannelForPackages(IEnumerable<string> packageIds) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(packageIds); | ||
|
|
||
| var requestedPackageIds = packageIds | ||
| .Where(packageId => !string.IsNullOrWhiteSpace(packageId)) | ||
| .Distinct(StringComparer.OrdinalIgnoreCase) | ||
| .ToArray(); | ||
|
|
||
| if (requestedPackageIds.Length == 0) | ||
| { | ||
| throw new ArgumentException("At least one package ID must be provided.", nameof(packageIds)); | ||
| } | ||
|
|
||
| var mappings = Mappings; | ||
| if (!VersionHelper.IsPrChannel(Name) || Type is not PackageChannelType.Explicit || mappings is not { Length: > 0 }) | ||
| { | ||
| return this; | ||
| } | ||
|
|
||
| var scopedMappings = mappings | ||
| .SelectMany(mapping => CreateScopedMappings(mapping, requestedPackageIds)) | ||
| .ToArray(); | ||
|
|
||
| return new PackageChannel(Name, Quality, scopedMappings, nuGetPackageCache, ConfigureGlobalPackagesFolder, CliDownloadBaseUrl, PinnedVersion); | ||
| } | ||
|
|
||
| private static IEnumerable<PackageMapping> CreateScopedMappings(PackageMapping mapping, IReadOnlyCollection<string> packageIds) | ||
| { | ||
| if (!IsScopedAspireMapping(mapping)) | ||
| { | ||
| yield return mapping; | ||
| yield break; | ||
| } | ||
|
|
||
| var scopedPackageIds = GetScopedPackageIds(mapping.Source, packageIds); | ||
|
|
||
| foreach (var scopedPackageId in scopedPackageIds) | ||
| { | ||
| yield return new PackageMapping(scopedPackageId, mapping.Source); | ||
| } | ||
| } | ||
|
|
||
| private static HashSet<string> GetScopedPackageIds(string source, IEnumerable<string> packageIds) | ||
| { | ||
| var resolvedPackageIds = new HashSet<string>(packageIds, StringComparer.OrdinalIgnoreCase); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| if (!Directory.Exists(source)) | ||
| { | ||
| return resolvedPackageIds; | ||
| } | ||
|
|
||
| var packageFiles = Directory.EnumerateFiles(source, "*.nupkg", SearchOption.TopDirectoryOnly) | ||
| .Select(GetPackageFileMetadata) | ||
| .OfType<PackageFileMetadata>() | ||
| .GroupBy(metadata => metadata.PackageId, StringComparer.OrdinalIgnoreCase) | ||
| .ToDictionary( | ||
| group => group.Key, | ||
| group => group.OrderByDescending(metadata => metadata.Version, SemVersion.PrecedenceComparer).First(), | ||
| StringComparer.OrdinalIgnoreCase); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More places to use the comparer |
||
|
|
||
| var packagesToProcess = new Queue<string>(resolvedPackageIds); | ||
|
|
||
| while (packagesToProcess.Count > 0) | ||
| { | ||
| var currentPackageId = packagesToProcess.Dequeue(); | ||
| if (!packageFiles.TryGetValue(currentPackageId, out var metadata)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| foreach (var dependencyPackageId in GetDependencyPackageIds(metadata.PackageFilePath)) | ||
| { | ||
| if (packageFiles.ContainsKey(dependencyPackageId) && resolvedPackageIds.Add(dependencyPackageId)) | ||
| { | ||
| packagesToProcess.Enqueue(dependencyPackageId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return resolvedPackageIds; | ||
| } | ||
|
|
||
| private static PackageFileMetadata? GetPackageFileMetadata(string packageFile) | ||
| { | ||
| var packageIdentity = TryGetPackageIdentityFromPackageFileName(packageFile); | ||
| if (packageIdentity is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return new PackageFileMetadata(packageIdentity.Value.PackageId, packageIdentity.Value.Version, packageFile); | ||
| } | ||
|
|
||
| private static IEnumerable<string> GetDependencyPackageIds(string packageFile) | ||
| { | ||
| try | ||
| { | ||
| using var archive = ZipFile.OpenRead(packageFile); | ||
| var nuspecEntry = archive.Entries.FirstOrDefault(entry => entry.FullName.EndsWith(".nuspec", StringComparison.OrdinalIgnoreCase)); | ||
| if (nuspecEntry is null) | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| using var stream = nuspecEntry.Open(); | ||
| var document = XDocument.Load(stream); | ||
| return document | ||
| .Descendants() | ||
| .Where(element => element.Name.LocalName == "dependency") | ||
| .Select(element => element.Attribute("id")?.Value) | ||
| .Where(id => !string.IsNullOrWhiteSpace(id)) | ||
| .Distinct(StringComparer.OrdinalIgnoreCase) | ||
| .Cast<string>() | ||
| .ToArray(); | ||
| } | ||
| catch (IOException) | ||
| { | ||
| return []; | ||
| } | ||
| catch (InvalidDataException) | ||
| { | ||
| return []; | ||
| } | ||
| catch (System.Xml.XmlException) | ||
| { | ||
| return []; | ||
|
Comment on lines
+324
to
+334
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be at least some debug level logging. |
||
| } | ||
| } | ||
|
|
||
| private static (string PackageId, SemVersion Version)? TryGetPackageIdentityFromPackageFileName(string packageFile) | ||
| { | ||
| var packageFileName = Path.GetFileNameWithoutExtension(packageFile); | ||
| if (string.IsNullOrWhiteSpace(packageFileName)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var separatorIndex = packageFileName.IndexOf('.'); | ||
| while (separatorIndex >= 0 && separatorIndex < packageFileName.Length - 1) | ||
| { | ||
| var versionCandidate = packageFileName[(separatorIndex + 1)..]; | ||
| if (SemVersion.TryParse(versionCandidate, SemVersionStyles.Strict, out var version)) | ||
| { | ||
| return (packageFileName[..separatorIndex], version); | ||
| } | ||
|
|
||
| separatorIndex = packageFileName.IndexOf('.', separatorIndex + 1); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private readonly record struct PackageFileMetadata(string PackageId, SemVersion Version, string PackageFilePath); | ||
|
|
||
| private static bool IsScopedAspireMapping(PackageMapping mapping) | ||
| { | ||
| return mapping.PackageFilter.StartsWith("Aspire", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(mapping.PackageFilter, PackageMapping.AllPackages, StringComparison.Ordinal); | ||
| } | ||
|
|
||
| public static PackageChannel CreateExplicitChannel(string name, PackageChannelQuality quality, PackageMapping[]? mappings, INuGetPackageCache nuGetPackageCache, bool configureGlobalPackagesFolder = false, string? cliDownloadBaseUrl = null, string? pinnedVersion = null) | ||
| { | ||
| return new PackageChannel(name, quality, mappings, nuGetPackageCache, configureGlobalPackagesFolder, cliDownloadBaseUrl, pinnedVersion); | ||
|
|
@@ -207,4 +376,4 @@ public static PackageChannel CreateImplicitChannel(INuGetPackageCache nuGetPacka | |
| // for broader templating options. | ||
| return new PackageChannel("default", PackageChannelQuality.Both, null, nuGetPackageCache); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.