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

Merged
merged 10 commits into from
Sep 27, 2024
Merged

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

merged 10 commits into from
Sep 27, 2024

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?
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.

@davidfowl
Copy link
Member Author

/azp run

@build-analysis build-analysis bot mentioned this pull request Sep 27, 2024
Open
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl davidfowl enabled auto-merge (squash) September 27, 2024 04:04
@radical
Copy link
Member

radical commented Sep 27, 2024

Should we additionally wait for Job host started in https:/radical/aspire/blob/e2830042b191c9f0239c1c69fc477c2a60d5dd72/tests/Aspire.Playground.Tests/ProjectSpecificTests.cs#L70-L77 ? I think there might be a slight race here in when MyAzureBlobTrigger: blobTrigger appears vs Job host started. And we end up getting connection refused because are not using http resilience on the client either (right?).

@captainsafia
Copy link
Member

Should we additionally wait for Job host started in https:/radical/aspire/blob/e2830042b191c9f0239c1c69fc477c2a60d5dd72/tests/Aspire.Playground.Tests/ProjectSpecificTests.cs#L70-L77 ? I think there might be a slight race here in when MyAzureBlobTrigger: blobTrigger appears vs Job host started.

Yep, this is a good hunch. We could add a check for the Job host started to see if that helps stabilize the tests.

@davidfowl
Copy link
Member Author

Retries will probably make it more stable.

@davidfowl davidfowl merged commit 25bbf05 into main Sep 27, 2024
5 of 8 checks passed
@davidfowl davidfowl deleted the davidfowl/aca-resource branch September 27, 2024 06:10
This was referenced Sep 27, 2024
Open
Closed
Closed
Open
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
4 participants
@davidfowl @radical @captainsafia @eerhardt