Make container runtime health check conditional based on resource requirements#10393
Make container runtime health check conditional based on resource requirements#10393
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR makes the container runtime health check in BuildImagesAsync conditional on whether any resources require a Dockerfile-based container build. It adds a helper method to detect those resources and includes unit tests covering several scenarios.
- Introduce
ResourcesRequireContainerRuntimehelper to detect resources with both container image and Dockerfile build annotations - Guard the existing runtime health check in
BuildImagesAsyncso it only runs when needed - Add unit tests for project-only, Dockerfile-only, mixed, empty, and container-without-Dockerfile cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Aspire.Hosting/Publishing/ResourceContainerImageBuilder.cs | Wrap the health check call in an if and introduce the helper method |
| tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageBuilderTests.cs | Add tests and supporting mocks for ResourcesRequireContainerRuntime |
Comments suppressed due to low confidence (2)
tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageBuilderTests.cs:385
- Consider adding a test case for a resource with only
DockerfileBuildAnnotation(noContainerImageAnnotation) to ensureResourcesRequireContainerRuntimebehaves as expected in that scenario.
[Fact]
src/Aspire.Hosting/Publishing/ResourceContainerImageBuilder.cs:393
- The PR description states only
DockerfileBuildAnnotationshould trigger the health check, yet this implementation also requiresContainerImageAnnotation. Please clarify if both annotations are required or update the check to only look forDockerfileBuildAnnotation.
resource.TryGetLastAnnotation<ContainerImageAnnotation>(out _) &&
| Assert.False(result, "Container resources without DockerfileBuildAnnotation should not require container runtime"); | ||
| } | ||
|
|
||
| private static bool TestResourcesRequireContainerRuntime(IEnumerable<IResource> resources) |
There was a problem hiding this comment.
[nitpick] The test suite duplicates the logic of ResourcesRequireContainerRuntime. To reduce duplication and keep tests in sync, consider making the production helper internal (with an InternalsVisibleTo attribute) so tests can call it directly.
tests/Aspire.Hosting.Tests/Publishing/ResourceContainerImageBuilderTests.cs
Outdated
Show resolved
Hide resolved
…inerImageBuilder Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
…ethod instead of duplicate test logic Co-authored-by: eerhardt <8291187+eerhardt@users.noreply.github.com>
6d2018b to
bb4064d
Compare
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
/backport to release/9.4 |
|
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16375400633 |
Currently, the container runtime health check is performed unconditionally before building any images, even when no resources actually need the container runtime. This causes unnecessary errors when building applications that only contain .NET projects without Dockerfile-based containers.
This PR makes the health check conditional by:
ResourcesRequireContainerRuntimethat checks if any resources have bothContainerImageAnnotationandDockerfileBuildAnnotationBuildImagesAsyncto only perform the container runtime health check when resources actually need itChanges Made
DockerfileBuildAnnotationrequire the container runtime health checkdotnet publish /t:PublishContainer) don't require the health checkTesting
Added unit tests covering:
This is a minimal, surgical change that prevents unnecessary container runtime health checks while maintaining all existing functionality.
Fixes #10392.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
westus.data.mcr.microsoft.comdotnet publish /home/REDACTED/work/aspire/aspire/tests/testproject/TestProject.ServiceA/TestProject.ServiceA.csproj --configuration Release /t:PublishContainer /p:ContainerRepository=servicea(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.