Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support for customizing container apps in ACA via the CDK #5470

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Added support for customizing container apps in ACA via the CDK #5470

wants to merge 10 commits into from
+3,252 −13

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Aug 28, 2024 •
edited
Loading

Description

  • Added Aspire.Hosting.Azure.ContainerApps. This exposes 3 APIs used to configure and customize container app resources.
  • Added deployment target support to project and container resources in the manifest writer. This allows developers to express that a project/container gets deployed using the nested resource type. This requires a branch of azd to wire up and test.

Fixes #5179

Relies on Azure/azure-dev#4286 in azd for making deployment work.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 28, 2024
@build-analysis build-analysis bot mentioned this pull request Aug 28, 2024
Open
3 tasks
@davidfowl davidfowl added area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Sep 9, 2024
@build-analysis build-analysis bot mentioned this pull request Sep 20, 2024
Open
3 tasks
@davidfowl davidfowl force-pushed the davidfowl/aca-resource branch 5 times, most recently from 78c9d21 to ea1b681 Compare September 25, 2024 14:44
@build-analysis build-analysis bot mentioned this pull request Sep 25, 2024
Open
2 tasks
@davidfowl
Added support for customizing container apps in ACA via the CDK
b0520ee 
- Added Aspire.Hosting.Azure.ContainerApps. This exposes 3 APIs used to configure and customize container app resources.
- Added deployment target support to project and container resources in the manifest writer. This allows developers to express that a project/container gets deployed using the nested resource type. This requires a branch of azd to wire up and test.
@davidfowl davidfowl marked this pull request as ready for review September 25, 2024 22:08
@davidfowl davidfowl changed the title Experiementing with PublishAsContainerApp and deployment targets Added support for customizing container apps in ACA via the CDK Sep 25, 2024
@davidfowl davidfowl requested review from mitchdenny and eerhardt and removed request for mitchdenny September 25, 2024 22:11
eerhardt
eerhardt reviewed Sep 26, 2024
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of nits and questions. But this is looking really good.

Aspire.sln Outdated Show resolved Hide resolved
Directory.Packages.props Outdated Show resolved Hide resolved
src/Aspire.Hosting/Publishing/ManifestPublishingContext.cs
Comment on lines +156 to +158
if (project.TryGetLastAnnotation<DeploymentTargetAnnotation>(out var deploymentTarget))
{
Writer.WriteString("type", "project.v1");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need some sort of generalization for versioning. Probably not today with this PR. But I wonder if this pattern is going hold going forward - "If Annotation X is on this project, that means it is version Y".

src/Aspire.Hosting/ApplicationModel/DeploymentTargetAnnotation.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Azure/AzureConstructResource.cs Show resolved Hide resolved
src/Aspire.Hosting.Azure.ContainerApps/AzureContanierAppsInfrastructure.cs Show resolved Hide resolved
src/Aspire.Hosting.Azure.ContainerApps/AzureContanierAppsInfrastructure.cs Show resolved Hide resolved
src/Aspire.Hosting.Azure.ContainerApps/AzureContanierAppsInfrastructure.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Azure.ContainerApps/AzureContanierAppsInfrastructure.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting.Azure.ContainerApps/AzureContanierAppsInfrastructure.cs Outdated Show resolved Hide resolved
eerhardt
eerhardt approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in and rock an roll on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerhardt eerhardt eerhardt approved these changes

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@vhvb1989 vhvb1989 Awaiting requested review from vhvb1989

Assignees
No one assigned
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide ability to configure the ACA resource properties through C# code
2 participants
@davidfowl @eerhardt