-
Notifications
You must be signed in to change notification settings - Fork 740
Create .aspire/settings.json with appHostPath during aspire new #12918
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 -- 12918Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12918" |
b4c7824 to
411f4b5
Compare
Co-authored-by: mitchdenny <[email protected]>
Co-authored-by: mitchdenny <[email protected]>
411f4b5 to
470694c
Compare
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 enhances the aspire new command to automatically create a .aspire/settings.json file with the appHostPath configuration, preventing the VS Code extension from prompting users for this setting immediately after project creation.
Key Changes
- Added automatic
.aspire/settings.jsonfile creation after successful template instantiation - Implemented AppHost project detection for both
.csprojand single-fileapphost.csformats - Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Aspire.Cli/Templating/DotNetTemplateFactory.cs |
Added CreateSettingsFileAsync method to create settings file with AppHost path, and IsValidSingleFileAppHostAsync helper to validate single-file AppHost projects |
tests/Aspire.Cli.Tests/Commands/NewCommandTests.cs |
Added NewCommand_CreatesSettingsJsonWithAppHostPath test to verify settings file creation with correct AppHost path |
| /// <summary> | ||
| /// Creates .aspire/settings.json file with appHostPath configuration if an AppHost project is found. | ||
| /// </summary> |
Copilot
AI
Nov 19, 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.
Missing XML documentation for the outputPath and cancellationToken parameters. According to the coding guidelines, all parameters for public and private methods should be documented when the method has a summary tag.
| catch (Exception) | ||
| { | ||
| // If settings file creation fails, don't fail the entire operation | ||
| // Just continue without creating the settings file |
Copilot
AI
Nov 19, 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 catch block silently swallows all exceptions without any logging. This makes debugging difficult if settings file creation fails. Consider logging the exception at a debug or warning level to aid troubleshooting while still maintaining the non-breaking behavior.
| catch (Exception) | |
| { | |
| // If settings file creation fails, don't fail the entire operation | |
| // Just continue without creating the settings file | |
| catch (Exception ex) | |
| { | |
| // If settings file creation fails, don't fail the entire operation | |
| // Just continue without creating the settings file | |
| AnsiConsole.MarkupLine("[yellow][[Warning]][/] Failed to create settings file: [bold]{0}[/]. Continuing without settings file.", ex.Message); | |
| AnsiConsole.WriteException(ex, ExceptionFormats.ShortenEverything | ExceptionFormats.ShowLinks); |
| // Just continue without creating the settings file | ||
| } | ||
| } | ||
|
|
Copilot
AI
Nov 19, 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.
Missing XML documentation for this method. According to the coding guidelines, internal APIs should have brief <summary> tags explaining what the code does.
| /// <summary> | |
| /// Checks whether the specified file is a valid Aspire single file app host. | |
| /// </summary> |
| catch | ||
| { | ||
| // If we can't read the file, it's not a valid candidate |
Copilot
AI
Nov 19, 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 catch block swallows all exceptions without logging. Consider logging the exception at debug level to aid troubleshooting, especially since file I/O operations can fail for various reasons (permissions, encoding issues, etc.).
| catch | |
| { | |
| // If we can't read the file, it's not a valid candidate | |
| catch (Exception ex) | |
| { | |
| // If we can't read the file, it's not a valid candidate | |
| AnsiConsole.MarkupLine($"[grey][[Debug]] Could not read file '{candidateFile.FullName}': {ex.Message}[/]"); |
| // Write the settings file | ||
| var jsonContent = JsonSerializer.Serialize(settings, JsonSourceGenerationContext.Default.JsonObject); | ||
| await File.WriteAllTextAsync(settingsFilePath, jsonContent, cancellationToken); |
Copilot
AI
Nov 19, 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 code duplicates the logic from ProjectLocator.CreateSettingsFileAsync (lines 422-436) which uses configurationService.SetConfigurationAsync to create the settings file. This approach would merge with existing settings if any exist. Consider extracting this into a shared helper method or using the existing IConfigurationService.SetConfigurationAsync to maintain consistency and avoid duplication.
| // Write the settings file | |
| var jsonContent = JsonSerializer.Serialize(settings, JsonSourceGenerationContext.Default.JsonObject); | |
| await File.WriteAllTextAsync(settingsFilePath, jsonContent, cancellationToken); | |
| // Write the settings file using IConfigurationService to ensure merging and consistency | |
| await configurationService.SetConfigurationAsync(settingsFilePath, settings, cancellationToken); |
Running
aspire newcreates a project but no.aspire/settings.jsonfile, causing the VS Code extension to immediately prompt forappHostPathconfiguration after opening the project.Changes
Modified
DotNetTemplateFactory.ApplyTemplateAsyncto create.aspire/settings.jsonafter successful template instantiation:CreateSettingsFileAsyncmethod that:.csprojand single-fileapphost.cs)GetAppHostInformationAsyncinfrastructure.aspire/settings.jsonwith relative path to AppHost projectJsonSourceGenerationContextfor formatted outputExample
Running
aspire new aspire-starter --name MyApp --output ./myappnow creates:Original prompt
aspire newshould create a .aspire/settings.json file withappHostPathset after creation #12523💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.