Refuse synthesized 'staging' channel on daily CLI builds - #16717
Refuse synthesized 'staging' channel on daily CLI builds#16717Mitch Denny (mitchdenny) wants to merge 5 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16717Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16717" |
There was a problem hiding this comment.
Pull request overview
Prevents aspire update --channel staging from silently resolving to daily packages when the running CLI is itself a daily build. The PR adds staging-unavailable detection in PackagingService, reuses that reason in UpdateCommand, and adds regression tests plus localized resource entries.
Changes:
- Added a new staging-unavailable reason API and daily-build heuristic in
PackagingService. - Updated
UpdateCommandto surface a staging-specific error instead of the generic missing-channel message. - Added regression tests and new packaging resource strings for the user-facing message.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Aspire.Cli.Tests/TestServices/TestPackagingService.cs |
Extended test fake to expose staging-unavailable reason. |
tests/Aspire.Cli.Tests/Packaging/PackagingServiceTests.cs |
Added regression coverage for daily/stable/blessed staging resolution cases. |
tests/Aspire.Cli.Tests/Commands/UpdateCommandTests.cs |
Added test for explicit staging error message. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.zh-Hant.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.zh-Hans.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.tr.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.ru.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.pt-BR.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.pl.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.ko.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.ja.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.it.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.fr.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.es.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.de.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/xlf/PackagingStrings.cs.xlf |
Added localized staging-unavailable string entry. |
src/Aspire.Cli/Resources/PackagingStrings.resx |
Added source resource string for staging-unavailable message. |
src/Aspire.Cli/Resources/PackagingStrings.Designer.cs |
Regenerated strongly-typed resource accessor. |
src/Aspire.Cli/Packaging/PackagingService.cs |
Added daily-build gating, test seam, logging, and staging-unavailable reason generation. |
src/Aspire.Cli/Commands/UpdateCommand.cs |
Switched missing-channel message construction to use staging-specific reason. |
Files not reviewed (1)
- src/Aspire.Cli/Resources/PackagingStrings.Designer.cs: Language not supported
|
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.
|
560b48f to
88af838
Compare
442c76a to
c4776de
Compare
Three issues raised in the Copilot review of #16717: 1. Asymmetric overrideStagingFeed validation. GetStagingChannelUnavailableReason short-circuited on any non-empty override, but GetStagingFeedUrl only honors it when it parses as an HTTP(S) URL via UrlHelper.IsHttpUrl. Setting overrideStagingFeed=foo (any non-URL) bypassed the daily-CLI guard, then the feed-URL builder silently fell back to the shared dotnet9 daily feed — reintroducing #16652. Match the validation in both call sites. 2. Test seam exposed via aspire config. The internal:packaging:cliVersionForTesting key was readable from IConfiguration, which aspire config set writes to (only legacy AppHost keys were blocked). Users could spoof CLI version with 'aspire config set internal:packaging:cliVersionForTesting 13.4.0' to bypass the daily-CLI guard in production. Add IsInternalConfigurationKey to AppHostPathConfigurationPolicy and reject internal: keys from both 'config set' and 'config delete'. Tests still drive the seam through IConfiguration directly so they are unaffected. 3. Self-update path. 'aspire update --self --channel staging' goes through CliDownloader.DownloadLatestCliAsync rather than ChannelNotFoundException, so it surfaced a generic 'Unsupported channel' error instead of the new staging reason. Consult GetStagingChannelUnavailableReason in ExecuteSelfUpdateAsync before any download work. Tests: 3036 passed / 0 failed / 20 skipped on Aspire.Cli.Tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On a daily Aspire CLI, `aspire update --channel staging` could resolve to daily packages because PackagingService.CreateStagingChannel() either built a per-commit darc-pub feed URL from the daily commit hash (no such feed exists) or fell back to the shared dotnet9 daily feed. Either way the user got daily packages instead of staging packages with no warning. PackagingService now refuses to synthesize the staging channel when the running CLI is itself a daily/CI build (detected via prerelease-identifier count on the assembly informational version) unless the user provides an explicit overrideStagingFeed configuration value. The resolved staging feed URL is also logged at info level so users can verify channel resolution. When the staging channel is omitted on a daily CLI, `aspire update --channel staging` now surfaces an actionable error explaining why and how to recover (set 'overrideStagingFeed' or use a stable CLI build). Fixes #16652 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nism The 8 pre-existing tests in PackagingServiceTests that exercise the shared-feed and pin-to-CLI-version paths assume the running CLI is not itself a daily/CI build, but they don't pin the version. With this PR's new daily-CLI guard, when CI builds the test assembly with a daily-flavored VersionSuffix (e.g. pr.NNNN.gSHA), the daily heuristic treats the running CLI as daily and GetStagingChannelUnavailableReason hides the staging channel, causing the tests to fail with "Sequence contains no matching element" on .First(c => c.Name == "staging"). Use the existing internal:packaging:cliVersionForTesting test seam to pin a stable CLI version (13.4.0) in those tests so the daily-CLI guard doesn't trigger and the tests deterministically exercise the shared-feed and pinning code paths regardless of the test-assembly's build flavor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Three issues raised in the Copilot review of #16717: 1. Asymmetric overrideStagingFeed validation. GetStagingChannelUnavailableReason short-circuited on any non-empty override, but GetStagingFeedUrl only honors it when it parses as an HTTP(S) URL via UrlHelper.IsHttpUrl. Setting overrideStagingFeed=foo (any non-URL) bypassed the daily-CLI guard, then the feed-URL builder silently fell back to the shared dotnet9 daily feed — reintroducing #16652. Match the validation in both call sites. 2. Test seam exposed via aspire config. The internal:packaging:cliVersionForTesting key was readable from IConfiguration, which aspire config set writes to (only legacy AppHost keys were blocked). Users could spoof CLI version with 'aspire config set internal:packaging:cliVersionForTesting 13.4.0' to bypass the daily-CLI guard in production. Add IsInternalConfigurationKey to AppHostPathConfigurationPolicy and reject internal: keys from both 'config set' and 'config delete'. Tests still drive the seam through IConfiguration directly so they are unaffected. 3. Self-update path. 'aspire update --self --channel staging' goes through CliDownloader.DownloadLatestCliAsync rather than ChannelNotFoundException, so it surfaced a generic 'Unsupported channel' error instead of the new staging reason. Consult GetStagingChannelUnavailableReason in ExecuteSelfUpdateAsync before any download work. Tests: 3036 passed / 0 failed / 20 skipped on Aspire.Cli.Tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7f869dd to
3c78194
Compare
|
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.
|
|
pr-testing skill? |
Removes the AppHostPathConfigurationPolicy.IsInternalConfigurationKey gate, the associated ErrorStrings resource (plus xlf entries), and the two ConfigCommandTests cases. The staging-channel daily-CLI guard and the override-feed validation (#1) and self-update short-circuit (#3) from commit 3c78194 are preserved. 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.
Matched test failure patterns (1 test)
|
|
❌ CLI E2E Tests failed — 84 passed, 1 failed, 1 unknown (commit Failed Tests
View all recordings
📹 Recordings uploaded automatically from CI run #25956711745 |
Description
Fixes #16652.
PackagingServicesynthesized thestagingchannel relative to the running CLI build:darc-pub-microsoft-aspire-{currentCliCommitHash}).When the running CLI is itself a daily build, neither produces a real staging feed: there is no SHA-specific darc feed for daily commits, and the shared daily feed contains daily packages. As a result
aspire update --channel stagingsilently resolved to daily versions — the bug tracked by #16652.This PR makes
stagingresolution deterministic on daily CLI builds by refusing to synthesize the channel rather than silently downgrading:IPackagingService.GetStagingChannelUnavailableReason()returns a localized, user-facing explanation when staging cannot be resolved.CreateStagingChannel()consults that helper. If staging is unavailable on a daily CLI it logs a warning and returnsnullinstead of fabricating a daily feed.UpdateCommandsurfaces the same reason in itsChannelNotFoundExceptionmessage when the user explicitly passes--channel staging, so the failure is clear and actionable.CreateStagingChannel()always logs the resolved feed URL + quality + pinned version when it does produce a channel (the "show what was resolved" suggestion from the issue).Escape hatches and unaffected paths:
overrideStagingFeedconfiguration entry continues to take ownership; staging is allowed in that case regardless of CLI build.preview.1,rc.1) continue to produce a real staging channel.preview.1.26210.1) are treated as daily.This is a 💥 blocking-release fix targeting
mainfirst; a backport PR torelease/13.3will follow.Validation
To verify locally, install the CLI from this PR:
Then:
A stable / blessed-preview CLI is unaffected — staging continues to resolve via the SHA-specific darc feed.
Automated coverage
tests/Aspire.Cli.Tests/Packaging/PackagingServiceTests.cs(new file, ~280 lines) covers staging behavior across stable / blessed-preview / daily CLI versions, with and withoutoverrideStagingFeed, including the unparseable-version fail-safe.tests/Aspire.Cli.Tests/Commands/UpdateCommandTests.cscovers--channel stagingsurfacing the staging-specific exception message instead of the generic "no channel matching" message.internal:packaging:cliVersionForTestingconfig key) lets unit tests deterministically simulate stable, blessed-preview, and daily CLI builds without depending on the actualAspire.Cli.dllassembly version. The key is internal/test-only, scoped toPackagingConfigurationKeys, and clearly documented as such.Checklist