Move Python starter template to TypeScript AppHost and CLI template factory#15574
Move Python starter template to TypeScript AppHost and CLI template factory#15574
Conversation
davidfowl
requested review from
JamesNK,
eerhardt,
mitchdenny and
radical
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15574Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15574" |
There was a problem hiding this comment.
Pull request overview
Migrates the Python starter template (aspire-py-starter) from the legacy dotnet new template pipeline to the embedded CLI template pipeline, switching the AppHost from C# to TypeScript to reduce required toolchains and align with the existing TS starter pattern.
Changes:
- Removed
aspire-py-starterfromDotNetTemplateFactory/ old project template assets and updated tests accordingly. - Added a new embedded CLI template tree for
py-starter(TypeScript AppHost + FastAPI backend + Vite React frontend). - Introduced
ConditionalBlockProcessor(with unit tests) and added a new CLI template factory implementation for the Python starter, including a--use-redis-cacheoption and conditional template processing.
Reviewed changes
Copilot reviewed 36 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Templates.Tests/LocalhostTldHostnameTests.cs | Removes py-starter coverage from dotnet-template hostname tests since it’s no longer a dotnet template. |
| tests/Aspire.Cli.Tests/Templating/DotNetTemplateFactoryTests.cs | Updates expectations so py-starter is no longer returned by DotNetTemplateFactory. |
| tests/Aspire.Cli.Tests/Templating/ConditionalBlockProcessorTests.cs | Adds unit coverage for the new conditional block processing utility. |
| src/Aspire.ProjectTemplates/Aspire.ProjectTemplates.csproj | Stops packaging the old py-starter dotnet template content. |
| src/Aspire.Cli/Aspire.Cli.csproj | Embeds the new py-starter template resources into the CLI. |
| src/Aspire.Cli/Templating/DotNetTemplateFactory.cs | Removes py-starter from dotnet template enumeration and associated prompt logic. |
| src/Aspire.Cli/Templating/KnownTemplateId.cs | Adds a PythonStarter known template ID constant. |
| src/Aspire.Cli/Templating/ConditionalBlockProcessor.cs | Adds reusable conditional block marker processing for templates. |
| src/Aspire.Cli/Templating/CliTemplateFactory.cs | Registers the Python starter in CLI template definitions and adds --use-redis-cache option plumbing. |
| src/Aspire.Cli/Templating/CliTemplateFactory.PythonStarterTemplate.cs | Implements the new CLI template application logic, conditional Redis inclusion, and aspire.config.json mutation. |
| src/Aspire.Cli/Templating/Templates/py-starter/** | Adds the new embedded Python starter template content (AppHost + backend + frontend). |
| src/Aspire.ProjectTemplates/templates/aspire-py-starter/** | Deletes the legacy dotnet-template assets for the Python starter. |
Files not reviewed (1)
- src/Aspire.Cli/Templating/Templates/py-starter/package-lock.json: Language not supported
| var ports = GenerateRandomPorts(); | ||
| var hostName = useLocalhostTld ? $"{projectNameLower}.dev.localhost" : "localhost"; | ||
| var conditions = new Dictionary<string, bool> | ||
| { | ||
| ["redis"] = useRedisCache, | ||
| ["no-redis"] = !useRedisCache, | ||
| }; | ||
| string ApplyAllTokens(string content) => ConditionalBlockProcessor.Process( | ||
| ApplyTokens(content, projectName, projectNameLower, aspireVersion, ports, hostName), |
There was a problem hiding this comment.
When --localhost-tld is enabled, the hostname is derived from projectName.ToLowerInvariant() without any DNS-safe normalization (e.g., replacing underscores/dots, trimming hyphens). Project names like Test_App.1 will produce invalid *.dev.localhost hostnames, unlike the .NET template behavior validated by LocalhostTldHostnameTests.
Consider normalizing the project name to a DNS-compliant label (replace invalid chars with hyphens, collapse repeats, trim leading/trailing hyphens) before appending .dev.localhost.
There was a problem hiding this comment.
This is a pre-existing pattern shared by all CLI templates — both CliTemplateFactory.TypeScriptStarterTemplate.cs (line 55) and CliTemplateFactory.EmptyTemplate.cs (line 167) use the same projectName.ToLowerInvariant() without DNS normalization. This PR just follows the established pattern. A DNS-safe normalization improvement could be done as a follow-up across all templates via the shared ApplyTokens method.
davidfowl
force-pushed
the
davidfowl/python-template-ts
branch
2 times, most recently
from
eb4b0e8 to
c190fc3
Compare
| while (true) | ||
| { | ||
| var startIdx = content.IndexOf(startPattern, StringComparison.Ordinal); | ||
| if (startIdx < 0) | ||
| { | ||
| break; |
There was a problem hiding this comment.
If a start marker exists but the matching end marker is misspelled or missing, the processor silently returns the content unchanged — leaving raw {{#name}} markers in the generated template output. This would be very hard to catch since there's no warning. Consider logging a warning (or throwing) when startIdx >= 0 but endIdx < 0, so template authoring typos don't produce silently broken output.
| } | ||
| } | ||
| catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) | ||
| { | ||
| _interactionService.DisplayError($"Failed to create project files: {ex.Message}"); | ||
| return new TemplateResult(ExitCodeConstants.FailedToCreateNewProject); | ||
| } | ||
|
|
||
| _interactionService.DisplaySuccess($"Created Python starter project at {outputPath.EscapeMarkup()}"); | ||
| DisplayPostCreationInstructions(outputPath); | ||
|
|
||
| return templateResult; | ||
| } | ||
|
|
||
| private async Task<bool> ResolveUseRedisCacheAsync(System.CommandLine.ParseResult parseResult, CancellationToken cancellationToken) | ||
| { | ||
| var redisCacheOptionSpecified = parseResult.Tokens.Any(token => | ||
| string.Equals(token.Value, "--use-redis-cache", StringComparisons.CliInputOrOutput)); | ||
| var useRedisCache = parseResult.GetValue(_useRedisCacheOption); | ||
| if (!redisCacheOptionSpecified) | ||
| { | ||
| if (!_hostEnvironment.SupportsInteractiveInput) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| useRedisCache = await _interactionService.PromptForSelectionAsync( | ||
| TemplatingStrings.UseRedisCache_Prompt, | ||
| [TemplatingStrings.Yes, TemplatingStrings.No], | ||
| choice => choice, | ||
| cancellationToken) switch | ||
| { | ||
| var choice when string.Equals(choice, TemplatingStrings.Yes, StringComparisons.CliInputOrOutput) => true, | ||
| var choice when string.Equals(choice, TemplatingStrings.No, StringComparisons.CliInputOrOutput) => false, | ||
| _ => throw new InvalidOperationException(TemplatingStrings.UseRedisCache_UnexpectedChoice) |
There was a problem hiding this comment.
This method is a near-duplicate of ResolveUseLocalhostTldAsync — same check-tokens-then-prompt-interactively pattern. Both check parseResult.Tokens for a specific option name, call parseResult.GetValue, fall back to PromptForSelectionAsync with Yes/No, and display a confirmation message. Consider extracting a shared helper like ResolveBooleanOptionAsync(parseResult, option, promptText, confirmationText, cancellationToken) to reduce duplication, especially since more templates with boolean options will likely follow.
| internal static string Process(string content, IReadOnlyDictionary<string, bool> conditions) | ||
| { | ||
| foreach (var (blockName, include) in conditions) | ||
| { | ||
| content = ProcessBlock(content, blockName, include); | ||
| } | ||
|
|
||
| return content; |
There was a problem hiding this comment.
Process() iterates IReadOnlyDictionary<string, bool> whose enumeration order is not contractually guaranteed. For the current redis/no-redis usage with non-overlapping blocks this works fine, but the API shape invites future callers to add conditions with nested or overlapping blocks, where the behavior would become silently order-dependent.
Minor suggestion: either document the non-overlapping requirement with a comment here, or accept an IReadOnlyList<(string Name, bool Include)> to make ordering explicit.
| content = ProcessBlock(content, blockName, include); | ||
| } | ||
|
|
||
| return content; |
There was a problem hiding this comment.
Add a debug assert that there are no conditional blocks in the content once complete? i.e. have a regex check for {{#ALPHA_CHARACTERS}} and {{/ALPHA_CHARACTERS}} and throw an error if there is any match
|
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.
|
|
This should use python apphost right? #13947 |
It can, but the app has javascript, python and C#. We're not shipping the python apphost yet. I suspect that will be a bigger move than adding another TS apphost. |
|
/azp run |
davidfowl
force-pushed
the
davidfowl/python-template-ts
branch
2 times, most recently
from
dd71c5a to
7b8fc68
Compare
davidfowl
requested review from
adamint,
danegsta,
karolz-ms and
sebastienros
as code owners
davidfowl
force-pushed
the
davidfowl/python-template-ts
branch
4 times, most recently
from
778a935 to
9650f92
Compare
Status: Blocked on bundle mode multi-package NuGet restoreThe template migration itself is complete and works locally (SDK mode). The E2E test currently only verifies template creation + SDK file generation. Blocker: Bundle mode (native AOT CLI) doesn't include integration assemblies in the generated TypeScript SDK when Evidence from CI workspace capture:
Once the bundle mode multi-package restore is fixed, the E2E test should be updated to include |
davidfowl
force-pushed
the
davidfowl/python-template-ts
branch
2 times, most recently
from
2d65132 to
8450db4
Compare
|
🎬 CLI E2E Test Recordings — 52 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23581790168 |
…actory - Migrate py-starter from DotNetTemplateFactory (C# AppHost) to CliTemplateFactory (TypeScript AppHost) - Add ConditionalBlockProcessor for template conditional blocks (redis support) - Add CaptureWorkspaceOnFailure attribute for CI workspace capture on test failure - Remove old dotnet new template from Aspire.ProjectTemplates - E2E test verifies template creation and SDK generation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/python-template-ts
branch
from
8450db4 to
9cb8071
Compare
Description
Migrates the
aspire-py-startertemplate from the olddotnet newtemplate system (DotNetTemplateFactory with C# AppHost) to the new embedded CLI template system (CliTemplateFactory with TypeScript AppHost).Motivation: Narrowing down the toolchains required — by moving to a TypeScript AppHost, the Python starter no longer requires the .NET SDK/C# toolchain for the AppHost project, aligning it with the same pattern as
aspire-ts-starter.What changed
src/Aspire.Cli/Templating/Templates/py-starter/with a TypeScript AppHost usingaddUvicornApp, conditional Redis support via{{#redis}}/{{/redis}}block markers, and a Vite React frontendCliTemplateFactory.PythonStarterTemplate.cs— factory logic for the Python starter, including Redis conditional processing andaspire.config.jsonpackage injectionConditionalBlockProcessorutility class — extracted reusable conditional block processing with 29 unit tests covering comment styles, edge cases, multiple conditions, and realistic template scenariosAspire.ProjectTemplates(dotnet new aspire-py-starteris replaced byaspire new aspire-py-starter)aspire-py-startermoved fromDotNetTemplateFactorytoCliTemplateFactorywith--use-redis-cacheand--localhost-tldoptionsDotNetTemplateFactoryTestsupdated to expect py-starter not in dotnet templates; hostname tests removed for py-starterValidation
Aspire.CliandAspire.ProjectTemplates)ConditionalBlockProcessorTestspassDotNetTemplateFactoryTestspassNewCommandTestspass (confirmsaspire-py-starterlisted in CLI help)aspire new aspire-py-startergenerates correct output for both--use-redis-cache falseand--use-redis-cache truevariantsChecklist
aspire.devissue: