-
Notifications
You must be signed in to change notification settings - Fork 756
[release/13.1] Fix: Execute container runtime health check on single-image builds #13579
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.
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
Conversation
- Updated ResourcesRequireContainerRuntimeAsync to skip runtime check for resources with Archive destination - Added runtime check to BuildImageAsync for Dockerfile resources - Updated FakeContainerRuntime test helper to support isRunning parameter - Added tests to verify the behavior (3 of 4 passing, 1 test needs refinement) Co-authored-by: captainsafia <[email protected]>
…ibility Reordered logic to check Dockerfile resources first, then Archive destination, preserving the original behavior for OCI + OutputPath combinations. Co-authored-by: captainsafia <[email protected]>
- Move container runtime check to beginning of BuildImageAsync for all resource types - Optimize ResourcesRequireContainerRuntimeAsync to call ResolveContainerBuildOptionsAsync once - Only check Destination when OutputPath is present to avoid unnecessary callback Co-authored-by: captainsafia <[email protected]>
- Added Destination property to ResolvedContainerBuildOptions class - Set Destination in ResolveContainerBuildOptionsAsync from context - Use Destination directly in ResourcesRequireContainerRuntimeAsync instead of calling ProcessContainerBuildOptionsCallbackAsync again - Simplifies logic and avoids redundant callback invocations Co-authored-by: captainsafia <[email protected]>
- Updated error log and exception messages to include container runtime name
- Changed from "Container runtime is not running" to "Container runtime '{name}' is not running"
- Updated test assertions to match new error message format
- Improves error diagnostics by showing which runtime (docker/podman) is being checked
Co-authored-by: captainsafia <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13579Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13579" |
|
@copilot Remove the |
|
@captainsafia I've opened a new pull request, #13580, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ions and improve error messages (#13580) * Initial plan * Remove ContainerImageDestination related parts as requested - Removed Destination property from ResolvedContainerBuildOptions - Reverted ResourcesRequireContainerRuntimeAsync to original logic - Removed 5 tests that used ContainerImageDestination enum - Reverted FakeContainerRuntime test helper changes - Kept error message improvements with container runtime name Co-authored-by: captainsafia <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: captainsafia <[email protected]>
joperezr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good later to think about how to best de-duplicate the code.
Backport of #13576 to release/13.1
/cc @captainsafia @copilot
Customer Impact
Makes sure the the container runtime check is in place when doing single-image builds so that users get clearer errors when their deployments will fail because Docker/Podman is not running.
Testing
Automated and manual.
Risk
Low.
Regression?
No.