-
Notifications
You must be signed in to change notification settings - Fork 708
Encapsulate SDK installation decision in CheckAsync #12367
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
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12367Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12367" |
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 refactors the SDK installation logic to encapsulate the installation decision within CheckAsync, making SDK installation automatic based on feature flags rather than prompting users. The changes replace a tuple-based return type with a structured CheckInstallResult class and simplify the installation flow by removing user confirmation prompts.
Key changes:
- Introduced
CheckInstallResultclass to provide structured return data from SDK checks - Moved installation decision logic into
CheckAsyncvia newDetermineShouldInstallmethod - Removed user prompting from
SdkInstallHelper.EnsureSdkInstalledAsyncand simplified its signature
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/DotNet/CheckInstallResult.cs | New class defining structured result for SDK check operations |
| src/Aspire.Cli/DotNet/IDotNetSdkInstaller.cs | Updated interface to return CheckInstallResult instead of tuple |
| src/Aspire.Cli/DotNet/DotNetSdkInstaller.cs | Refactored to return CheckInstallResult, added DetermineShouldInstall method, and added ICliHostEnvironment parameter |
| src/Aspire.Cli/Utils/SdkInstallHelper.cs | Simplified to remove IFeatures and ICliHostEnvironment parameters and user confirmation prompt logic |
| src/Aspire.Cli/Commands/*.cs | Updated all command classes to use simplified EnsureSdkInstalledAsync signature |
| src/Aspire.Cli/NuGet/NuGetPackagePrefetcher.cs | Updated to use CheckInstallResult properties instead of tuple deconstruction |
| tests/Aspire.Cli.Tests/DotNetSdkInstallerTests.cs | Updated tests to create CheckInstallResult objects and use property access instead of tuple deconstruction |
| tests/Aspire.Cli.Tests/Commands/SdkInstallerTests.cs | Updated test factories to create CheckInstallResult objects |
| tests/Aspire.Cli.Tests/TestServices/TestDotNetSdkInstaller.cs | Updated mock to return CheckInstallResult instead of tuple |
| /// <summary> | ||
| /// Determines whether the SDK should be installed automatically based on feature flags and environment capabilities. | ||
| /// </summary> |
Copilot
AI
Oct 25, 2025
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.
The XML documentation for DetermineShouldInstall should include a <remarks> section explaining the conditions under which installation occurs. Consider adding: <remarks>Installation occurs when: (1) The SDK check failed or force install is enabled, (2) The DotNetSdkInstallationEnabled feature is enabled, and (3) The environment supports interactive input or force install is enabled.</remarks>
| /// Checks if an installed SDK version meets the minimum requirement. | ||
| /// For .NET 10.x requirements, allows any .NET 10.x version including prereleases. | ||
| /// </summary> |
Copilot
AI
Oct 25, 2025
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.
The summary documentation for MeetsMinimumRequirement is incomplete and should be updated to reflect the current behavior. It should state: 'Checks if an installed SDK version meets the minimum requirement using strict semantic version comparison.'
Updated test files to use the new CheckInstallResult object instead of tuple deconstruction: - Fixed all test cases in SdkInstallerTests.cs to create CheckInstallResult objects - Fixed all test cases in DotNetSdkInstallerTests.cs to access result properties - Updated reflection-based tests to match simplified MeetsMinimumRequirement implementation - All 486 tests now pass successfully Co-authored-by: mitchdenny <[email protected]>
e5c6f84 to
f29d94f
Compare
Refactors SDK installation logic to move the decision of whether to install into
CheckAsyncitself, removing user prompts and making installation automatic based on feature flags.Changes
New
CheckInstallResultclass - Replaces tuple return with structured result:DotNetSdkInstaller.CheckAsync- Now determines installation automatically:CheckInstallResultinstead of tupleShouldInstallbased on feature flags and environment capabilitiesSdkInstallHelper.EnsureSdkInstalledAsync- Simplified to remove prompting:IFeaturesorICliHostEnvironmentparametersresult.ShouldInstallis trueCommand updates - All commands now use simplified signature:
Test updates - Fixed compilation errors from tuple → object change:
CheckInstallResultobjectsMeetsMinimumRequirementsignatureOriginal prompt
Create a coding agent PR but don't make any changes beyond my modifications here. Once we've got a build running I'll suggest additional changes.
[Chronological Review: The conversation began with the user requesting changes to the `SdkInstallHelper` and `DotNetSdkInstaller.CheckAsync` methods to encapsulate the SDK installation logic within `CheckAsync`. The user later clarified that the installation should be automatic without prompting the user. Finally, the user requested the creation of a pull request (PR) for the modifications made, indicating that no further changes should be made until a build is running.][Intent Mapping:
[Technical Inventory:
SdkInstallHelper.cs: The file being modified to change SDK installation logic.CheckAsync: Method withinDotNetSdkInstallerthat needs to be updated to return aCheckInstallResultobject.CheckInstallResult: New object structure to include installation determination logic.][Code Archaeology:
SdkInstallHelper.cs: The user is focused on modifying this file to implement the new logic for SDK installation.CheckAsync: Needs to be updated to include aShouldInstallvalue in its return type.][Progress Assessment:
SdkInstallHelperandCheckAsync.[Context Validation: All necessary context for continuing work on the
SdkInstallHelperandCheckAsyncmethods is captured, including the requirement for automatic installation without user prompts.][Recent Commands Analysis:
SdkInstallHelper.cs.1. Conversation Overview: - Primary Objectives: - "Change the logic in SdkInstallHelper and DotNetSdkInstaller.CheckAsync so that the determination on whether the SDK should be installed is made within CheckAsync itself." - "Change the CheckAsync method to return an object which is CheckInstallResult which has the values from the tuple along with a ShouldInstall value." - "I forgot to mention we no longer want to prompt to install. The determination on whether we should install will be automatic based on the checks inside CheckAsync." - "Create a coding agent PR but don't make any changes beyond my modifications here. Once we've got a build running I'll suggest additional changes." - Session Context: The user is modifying SDK installation logic in the `SdkInstallHelper` and `DotNetSdkInstaller` classes, focusing on automatic installation without user prompts. - User Intent Evolution: The user initially requested specific changes to the installation logic and later clarified the need for automatic behavior without prompts.- Technical Foundation:
- Codebase Status:
- Purpose: Contains logic for SDK installation.
- Current State: Awaiting modifications as per user requests.
- Key Code Segments: Focus on
- Dependencies: Related to the installation process of the SDK.
- Problem Resolution:
- Issues Encountered: Need for a change in logic to automate SDK installation.
- Solutions Implemented: User has provided clear instructions for the required changes.
- Debugging Context: No ongoing troubleshooting efforts mentioned.
- Lessons Learned: Importance of clear communication regarding user prompts and installation logic.
- Progress Tracking:
- Completed Tasks: User has specified the changes neede...
SdkInstallHelper.cs: The file being modified to implement new logic for SDK installation.CheckAsync: Method that needs to be updated to return aCheckInstallResultobject.CheckInstallResult: New object structure to include installation determination logic.SdkInstallHelper.cs:CheckAsyncmethod changes.Created from VS Code via the GitHub Pull Request extension.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.