Generate ACA app infrastructure #2965
Draft
Conversation
This file contains 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
dotnet-issue-labeler
bot
added
the
area-codeflow
mitchdenny
reviewed
Mar 17, 2024
| var resourceToken = uniqueString(resourceGroup().id) | ||
|
|
||
| resource logAnalyticsWorkspace 'Microsoft.OperationalInsights/workspaces@2022-10-01' = { | ||
| name: 'law-${resourceToken}' |
There was a problem hiding this comment.
For these "global" resources I wonder if we need some concept of the app name which we can then use when generating these resource names.
mitchdenny
reviewed
Mar 17, 2024
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureContainerAppsInfastructure.cs
Show resolved
Hide resolved
mitchdenny
reviewed
Mar 17, 2024
...ground/AzureStorageEndToEnd/AzureStorageEndToEnd.AppHost/aspire-dashboard-containerapp.bicep
Outdated
Show resolved
Hide resolved
davidfowl
commented
Mar 17, 2024
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureContainerAppsInfastructure.cs
Outdated
Show resolved
Hide resolved
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
2 times, most recently
from
7dddef8
to
0f0cd91
Compare
| // REVIEW: Is this correct? | ||
| var anyHttp2 = http.Any(e => e.Transport == "http2") || https.Any(e => e.Transport == "http2"); | ||
| var external = http.Any(e => e.IsExternal) || https.Any(e => e.IsExternal); | ||
| var containerPort = resource is ProjectResource ? 8080 : 80; |
There was a problem hiding this comment.
Thoughts on how we'd support <ContainerPort>? https://learn.microsoft.com/en-us/dotnet/core/docker/publish-as-container?pivots=dotnet-8-0#containerport
Is that something AZD will need to check for when it runs MSBuild or should our targets be injecting that value in to the assembly so we can get at it?
There was a problem hiding this comment.
Yes this will need some thought. It can either be passed into the app or declared on the endpoint (which is probably cleaner but duplicative).
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureContainerAppsInfastructure.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureContainerAppsInfastructure.cs
Outdated
Show resolved
Hide resolved
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
from
82c7311
to
14a2647
Compare
| // ACA can't handle > 5 additional ports so throw if that's the case here | ||
| if (endpointsByContainerPort.Count > 5) | ||
| { | ||
| throw new NotSupportedException("More than 5 additional ports are not supported. See https://learn.microsoft.com/en-us/azure/container-apps/ingress-overview#tcp for more details."); |
There was a problem hiding this comment.
Suggested change
| throw new NotSupportedException("More than 5 additional ports are not supported. See https://learn.microsoft.com/en-us/azure/container-apps/ingress-overview#tcp for more details."); | |
| throw new NotSupportedException("More than 5 additional ports are not supported. See https://learn.microsoft.com/azure/container-apps/ingress-overview#tcp for more details."); |
| } | ||
|
|
||
| // Add additional ports | ||
| // https://learn.microsoft.com/en-us/azure/container-apps/ingress-how-to?pivots=azure-cli#use-additional-tcp-ports |
There was a problem hiding this comment.
Suggested change
| // https://learn.microsoft.com/en-us/azure/container-apps/ingress-how-to?pivots=azure-cli#use-additional-tcp-ports | |
| // https://learn.microsoft.com/azure/container-apps/ingress-how-to?pivots=azure-cli#use-additional-tcp-ports |
src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureContainerAppsInfastructure.cs
Outdated
Show resolved
Hide resolved
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
2 times, most recently
from
1f2a174
to
bcd43f6
Compare
mitchdenny
reviewed
Mar 25, 2024
| if (executionContext.IsPublishMode) | ||
| { | ||
| // TODO: Let the user pick what this is, for now just hard code azure container apps | ||
| await new AzureContainerAppsInfastructure(executionContext).GenerateAdditionalInfrastructureAsync(appModel, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
I'm thinking that we'll want a model where we can choose per app/resource. This is fine for a x.1 release though. Just pointing to the horizon from here.
There was a problem hiding this comment.
We'll need to enable that for functions, for sure, but that can't be the default mode.
mitchdenny
reviewed
Mar 25, 2024
| @@ -9,6 +9,9 @@ param keyVaultName string | |||
| @description('') | |||
| param principalId string | |||
|
|
|||
| @description('') | |||
There was a problem hiding this comment.
Need to regen your manifests after my merge of #3135
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
2 times, most recently
from
e54bb3b
to
54b2c7c
Compare
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
from
6c8ebbd
to
cdae1db
Compare
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
from
64849de
to
9298ebb
Compare
Regen manifest Prep for multiple tcp ports PR feedback Rules? Fixes Moar fixes Made the endpoint mapping more robust - Added container app context that has a reference to all of the processing contexts. It will handle caching processing contexts. - Resovle host and port based on endpoint mappings Fix bug with additional mapping Move everything to the contexts - Rename ProcessingContext to ContainerAppContext. Make everything private Fixed the formatting More gen Better formatting Full bicep gen Better parameter names More gen Updates Fix formatting Pick the first one Don't throw if there are more than 5 additional endpoints Do port allocation in the bicep generation Fixes Order by group index. Fix and skip a test Fixed test Added target port resolution Fix AI resource Fixed AppInsights issues, deleted old files
davidfowl
force-pushed
the
davidfowl/gen-compute-infra
branch
from
9298ebb
to
39c458c
Compare
- Bring to parity with azd logic
1 task
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.
Making progress on Azure/azure-dev#3292
Microsoft Reviewers: Open in CodeFlow