Export WithAdminDeploymentScriptSubnet for polyglot app hosts#15564
Open
mitchdenny wants to merge 1 commit intomainfrom
Open
Export WithAdminDeploymentScriptSubnet for polyglot app hosts#15564mitchdenny wants to merge 1 commit intomainfrom
mitchdenny wants to merge 1 commit intomainfrom
Conversation
Now that Aspire.Hosting.Azure.Network is shipped, AzureSubnetResource is available to polyglot app hosts. Replace [AspireExportIgnore] with [AspireExport] on WithAdminDeploymentScriptSubnet so TypeScript (and other polyglot) app hosts can configure explicit subnets for SQL Server admin deployment scripts. - Add [AspireExport] attribute with camelCase name and description - Update TypeScript Azure SQL validation app to exercise VNet + subnet + withAdminDeploymentScriptSubnet (validated by polyglot CI workflow) - Add TypeScript VNet SQL Server deployment E2E test Fixes #15373 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15564Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15564" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the AzureSqlExtensions.WithAdminDeploymentScriptSubnet capability available to polyglot app hosts (TypeScript/Python/etc.) by exporting it for code generation, and adds validation coverage proving the exported surface compiles and can be exercised in a TypeScript AppHost deployment scenario.
Changes:
- Export
WithAdminDeploymentScriptSubnetvia[AspireExport]so it’s available to polyglot SDKs. - Update the TypeScript Azure SQL polyglot validation AppHost to compile usage of VNet + subnet +
withAdminDeploymentScriptSubnet. - Add a new deployment E2E test that provisions a TypeScript AppHost with VNet + SQL + Private Endpoint and explicitly sets the deployment script subnet.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Aspire.Deployment.EndToEnd.Tests/TypeScriptVnetSqlServerInfraDeploymentTests.cs | Adds an Azure deployment E2E test covering a TypeScript AppHost scenario that uses withAdminDeploymentScriptSubnet. |
| src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs | Exports WithAdminDeploymentScriptSubnet for polyglot codegen. |
| playground/polyglot/TypeScript/Aspire.Hosting.Azure.Sql/aspire.config.json | Adds the Azure Network package dependency needed for VNet/subnet APIs in the TS validation host. |
| playground/polyglot/TypeScript/Aspire.Hosting.Azure.Sql/ValidationAppHost/apphost.ts | Exercises withAdminDeploymentScriptSubnet in the TS validation AppHost to ensure codegen compiles. |
Comment on lines
+392
to
394
| [AspireExport("withAdminDeploymentScriptSubnet", Description = "Configures the Azure SQL server to use a specific subnet for deployment scripts")] | ||
| [Experimental("ASPIREAZURE003", UrlFormat = "https://aka.ms/dotnet/aspire/diagnostics#{0}")] | ||
| public static IResourceBuilder<AzureSqlServerResource> WithAdminDeploymentScriptSubnet( |
There was a problem hiding this comment.
The XML docs for WithAdminDeploymentScriptSubnet still state "This method is not available in polyglot app hosts." but the method is now exported via [AspireExport]. Please remove or update that remark so the documentation matches the new behavior.
Contributor
|
🎬 CLI E2E Test Recordings — 49 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23521368161 |
Member
Author
|
/deployment-test |
Contributor
|
🚀 Deployment tests starting on PR #15564... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
Contributor
|
❌ Deployment E2E Tests failed — 24 passed, 5 failed, 0 cancelled View test results and recordings
|
Member
|
Will this conflict with #15557? cc @sebastienros |
Member
Author
|
Yeah they both do the same thing regarding the subnet resource, but this one might do a little more. Happy to wait until Seb's goes in and then rebase this one once that goes in. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Removes the
[AspireExportIgnore]attribute fromWithAdminDeploymentScriptSubnetand replaces it with[AspireExport], making this method available to polyglot (TypeScript, Python, etc.) app hosts.Background: The method was originally blocked because
AzureSubnetResourcefromAspire.Hosting.Azure.Networkwas not yet shipped. Now that the Network package is included in the Aspire release, the restriction should be lifted (as identified in #13639 review).Changes:
[AspireExportIgnore]with[AspireExport("withAdminDeploymentScriptSubnet", ...)]onAzureSqlExtensions.WithAdminDeploymentScriptSubnetwithAdminDeploymentScriptSubnet(proves codegen works and compiles)TypeScriptVnetSqlServerInfraDeploymentTestsdeployment E2E test for TypeScript app host with VNet + SQL + PE + explicit deployment script subnetFixes #15373
Checklist