Stop false Aspire CLI upgrade warning in VS Code Extension#18358
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18358Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18358" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR fixes #18337, where the Aspire VS Code panel showed a "Update Aspire CLI" upgrade prompt for any AppHosts fetch error — even when the CLI was already current. The change threads a new "compatibility error" flag through the extension's AppHostDataRepository error state so that only genuine CLI/AppHost version-compatibility failures show the upgrade-focused welcome message, while generic fetch/runtime failures show a new neutral message. A new aspire.fetchAppHostsCompatibilityError context key drives the viewsWelcome rendering split.
Changes:
AppHostDataRepositorynow distinguishes compatibility errors from generic errors (_getDescribeNoDataErrorreturns a{ message, isCompatibilityError }object;_setDescribeError/_setPsErroraccept acompatibilityoption) and publishes a newaspire.fetchAppHostsCompatibilityErrorcontext.package.jsonsplits the appHosts error welcome into a compatibility (upgrade) welcome and a new generic welcome; newgenericErrorWelcomestring added topackage.nls.json(andstrings.ts).- Unit tests updated to assert the new compatibility context behavior and manifest welcome conditions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
extension/src/views/AppHostDataRepository.ts |
Adds compatibility-error tracking and sets the new context key; correctly wires the new return type and per-error compatibility flags. |
extension/package.json |
Splits error welcome into compatibility vs. generic when clauses — but the corresponding E2E manifest assertion was not updated. |
extension/package.nls.json |
Adds the views.appHosts.genericErrorWelcome manifest string. |
extension/src/loc/strings.ts |
Adds appHostsGenericErrorWelcome, which is unused and drifts from the manifest text. |
extension/src/test/packageManifest.test.ts |
Adds substring assertions for the two new welcome when clauses. |
extension/src/test/appHostDataRepository.test.ts |
Adds assertions that the compatibility context is set true for version errors and false for generic errors. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
adamint
left a comment
There was a problem hiding this comment.
(automated review)
I think this still misses the path that looks closest to #18337. The package.json split works if aspire.fetchAppHostsCompatibilityError is false, but _getDescribeNoDataError() still marks any non-zero aspire describe --follow no-data exit with a selected workspace AppHost as compatibility at extension/src/views/AppHostDataRepository.ts:893. So runtime failures before any JSON is produced still drive the upgrade welcome.
I proved the before/after manifest behavior locally: pre-fix base shows %views.appHosts.errorWelcome% with [Update Aspire CLI] for fetchAppHostsError=true, while PR head shows %views.appHosts.genericErrorWelcome% when fetchAppHostsCompatibilityError=false. But using the PR-head classifier with runtime stderr like No container runtime detected gives isCompatibilityError=true, and the visible welcome is still %views.appHosts.errorWelcome% with [Update Aspire CLI]. That seems like the original false-upgrade symptom can still happen for a current CLI/AppHost when describe exits non-zero for a non-compatibility reason.
Could we restrict compatibility here to explicit unsupported-output/version signals, or at least not treat every non-zero no-data describe exit as an AppHost-version compatibility failure?
Keep non-zero describe exits on the generic error welcome path unless the CLI output explicitly indicates unsupported describe behavior. Preserve the compatibility guidance for empty successful describe streams after ps has observed the AppHost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
(automated comment) Fixed this in Proof I reran locally:
|
adamint
left a comment
There was a problem hiding this comment.
(automated review)
Looks good after cde4ef4b32. I re-proved the original false-upgrade path on the pre-fix base, then verified the fixed runtime-failure path now keeps aspire.fetchAppHostsCompatibilityError=false and shows the generic AppHost load error instead of the CLI upgrade CTA.
I also reran the targeted extension-host coverage for AppHostDataRepository + packageManifest (85 passing), plus compile-tests and lint. The remaining blocker is not this fix: GitHub still reports the PR merge state as DIRTY, so it needs conflict resolution before it can merge.
Resolve the AppHostDataRepository conflict by keeping both the post-stop refresh timer state from main and the describe no-data compatibility classifier from this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. Test projects (0 / 97) none — no .NET test projects run for this change. Jobs (2)
Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Description
The Aspire panel was showing upgrade guidance for any AppHosts fetch error, which made the workspace view ask users to update the CLI even when the CLI was already current.
This change separates compatibility failures from generic fetch/runtime failures in the VS Code extension data repository, then uses that distinction in viewsWelcome rendering:
Very short validation note: I created a simple AppHost scenario and exercised both a current CLI path and an intentionally bad/incompatible CLI path; the upgrade guidance now only appears for the compatibility case.
Fixes: #18337
Checklist
<remarks />and<code />elements on your triple slash comments?