-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove Aspire Workload and Add Deprecation Warning for .NET 10 SDK #49534
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes support for the Aspire workload, introduces a deprecation warning for any manual Aspire install attempts, and updates related tests, manifests, and localized resources.
- Removed Aspire entries from workload manifests, layout targets, version props, and CLI string resources.
- Updated existing tests to drop Aspire expectations and added new tests verifying the deprecation warning.
- Enhanced
WorkloadInstallCommand
to filter out "aspire" and emit a one-time deprecation message.
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/Commands/Workload/Install/WorkloadInstallCommand.cs | Added logic to skip "aspire" and show deprecation |
src/Cli/dotnet/Commands/CliCommandStrings.resx | Added new resource string for Aspire deprecation |
src/Layout/redist/targets/BundledManifests.targets | Removed Aspire workload from bundled manifests |
eng/Versions.props | Removed Aspire feature band and manifest version |
test/dotnet.Tests/.../GivenDotnetWorkloadUpdate.cs | Updated workload update tests to drop Aspire |
test/dotnet.Tests/.../WorkloadInstallAspireDeprecationTests.cs | Added tests for Aspire deprecation warning |
test/dotnet-MsiInstallation.Tests/... | Replaced Aspire references with wasm-tools in tests |
src/Cli/dotnet/Commands/xlf/CliCommandStrings.*.xlf | Inserted new translation stubs for deprecation msg |
documentation/general/workloads/workload-diagnosis.md | Removed outdated Aspire mention from docs |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Commands/Workload/Install/WorkloadInstallCommand.cs:72
- [nitpick] Consider renaming this method to reflect its side effects (filtering IDs and logging a deprecation message), e.g.,
FilterWorkloadIdsAndShowDeprecation
, to make its intent clearer.
private IReadOnlyCollection<string> GetValidWorkloadIds()
@@ -103,7 +132,8 @@ public override int Execute() | |||
// which require new packs for currently installed workloads, those packs will be downloaded. | |||
// If the packs are already installed, they won't be included in the results | |||
var existingWorkloads = GetInstalledWorkloads(false); | |||
var workloadsToDownload = existingWorkloads.Union(_workloadIds.Select(id => new WorkloadId(id))).ToList(); | |||
var filteredWorkloadIds = GetValidWorkloadIds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] To avoid redundant enumeration and improve clarity, consider calling GetValidWorkloadIds()
once at the start of Execute()
and reusing that list for validation, download, and installation steps.
var filteredWorkloadIds = GetValidWorkloadIds(); |
Copilot uses AI. Check for mistakes.
@@ -2403,6 +2402,9 @@ To display a value, specify the corresponding command-line option without provid | |||
<data name="WorkloadNotRecognized" xml:space="preserve"> | |||
<value>Workload ID {0} is not recognized.</value> | |||
</data> | |||
<data name="AspireWorkloadDeprecated" xml:space="preserve"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider generalizing this in case we ever need to deprecate other workloads and parameterize the workload ID and support policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to change it if you and @marcpopMSFT think it's the right thing to do, but my 2 cents are that I think that a) it will be relatively rare for us to deprecate workloads and b) when we do, we'll very likely want a specific and custom message.
…point them to docs to upgrade to new aspire sdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, not going to block on generalizing it. If we ever do a second removal we can generalize this to avoid having special cases.
Fixes #49278
Eliminate the Aspire workload and its associated manifest. Introduce a warning message for users attempting to manually install the Aspire workload, guiding them to alternative options.
Here is the experience of running a few of the workload commands after these changes:
cc: @baronfel, @marcpopMSFT, @DamianEdwards, @davidfowl, @maddymontaquila