Support daily channels for specific previews#54579
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support in dotnetup for “phase-qualified” daily SDK channels (for example 11.0.1xx-preview.5-daily / 11.0.1xx-preview5-daily), ensuring both matching behavior and aka.ms daily resolution work with the dotless path format used by the service.
Changes:
- Extended
UpdateChanneldaily matching to optionally require a specific prerelease label (e.g.,preview.5,rc.1) in addition to matching the base scope. - Updated
DailyChannelResolverto translate dotted prerelease labels into the dotless aka.ms path segment (e.g.,preview.5→preview5). - Added/updated test coverage for phase-qualified daily channels across match/validation/resolution.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/dotnetup.Tests/UpdateChannelTests.cs | Adds match and IsDaily cases for phase-qualified daily channels (preview.5, rc.1, dotless variants). |
| test/dotnetup.Tests/DailyChannelResolverTests.cs | Verifies phase-qualified daily channels resolve using the dotless aka.ms path segment. |
| test/dotnetup.Tests/ChannelVersionResolverTests.cs | Extends validation coverage; adds phase-qualified daily channels to the live aka.ms daily resolution theory. |
| src/Installer/Microsoft.Dotnet.Installation/Internal/UpdateChannel.cs | Implements parsing/normalization of prerelease labels and applies it to daily-channel matching. |
| src/Installer/Microsoft.Dotnet.Installation/Internal/DailyChannelResolver.cs | Converts prerelease-qualified daily channels into the aka.ms dotless path form. |
| src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs | Updates daily-channel format validation to allow prerelease-qualified daily scopes. |
| if (UpdateChannel.TrySplitPartialVersionAndPrereleaseLabel(basePart, out var bandPart, out _)) | ||
| { | ||
| return !string.IsNullOrEmpty(versionPart) && IsValidPartialVersion(versionPart); | ||
| return IsValidPartialVersion(bandPart); |
There was a problem hiding this comment.
Is this doing validation on baseaPart or should we also be checking that? I think that IsValidPartialVersion no longer executes on anything besides the band part, so it will pass as long as there is a - and valid version afterward.
| return IsValidPartialVersion(bandPart); | |
| return IsValidPartialVersion(bandPart) && isValidPartialVersion(getMajorMinor(basePart)); |
Suggestion 2:
add a unit test for input such as invalid.invalid.1xx-daily
There was a problem hiding this comment.
I also recognize this function appears to be a 'best-effort' implementation
| var dashIndex = channel.IndexOf('-', StringComparison.Ordinal); | ||
| if (dashIndex >= 0) | ||
| { | ||
| var versionPart = channel.Substring(0, dashIndex); |
There was a problem hiding this comment.
I don't think we validate the version component after the dash, so it might try to install a version string such as '10.0.1xx-RANDOM` - but don't think this is new. It likely gets caught elsewhere in the code? Could be worth improving or adding a test for now.
| if (UpdateChannel.TrySplitPartialVersionAndPrereleaseLabel(basePart, out var bandPart, out _)) | ||
| { | ||
| return !string.IsNullOrEmpty(versionPart) && IsValidPartialVersion(versionPart); | ||
| return IsValidPartialVersion(bandPart); |
There was a problem hiding this comment.
I also recognize this function appears to be a 'best-effort' implementation
| : channelName; | ||
|
|
||
| /// <summary> | ||
| /// Tries to split a daily-scope string into a partial-version prefix and a |
There was a problem hiding this comment.
Does daily-scope string mean a string that is a not fully specified version, or a fully specified version, but one without the daily suffix?
| private static string EnsureFeatureBand(string partialVersion) | ||
| { | ||
| var parts = partialVersion.Split('.'); | ||
| return parts.Length == 2 ? $"{partialVersion}.1xx" : partialVersion; |
There was a problem hiding this comment.
Is this the right assumption - e.g., : https://aka.ms/dotnet/10.0.4xx/daily this is a valid outcome and if I ask for 10.0-daily I'd expect to get 4xx. It might help to demonstrate the type of version input to this function - is it only 11.0-preview.5-daily? If this is true, it could be renamed to AddFeatureBandToPreviewVersion so it isn't misused.
|
Any chance that was you trying this version string out .... 😁 |
Support channels such as
11.0.1xx-preview.5-daily.Example: