-
Notifications
You must be signed in to change notification settings - Fork 758
Fix: Skip container runtime check for Archive destinations and improve error messages #13576
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
Changes from 5 commits
e016b73
c7a99c9
664f15c
0c62baf
82bdd4b
e25852e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -172,6 +172,7 @@ private sealed class ResolvedContainerBuildOptions | |||||
| public string? OutputPath { get; set; } | ||||||
| public ContainerImageFormat? ImageFormat { get; set; } | ||||||
| public ContainerTargetPlatform? TargetPlatform { get; set; } | ||||||
| public ContainerImageDestination? Destination { get; set; } | ||||||
| public string LocalImageName { get; set; } = string.Empty; | ||||||
| public string LocalImageTag { get; set; } = "latest"; | ||||||
| } | ||||||
|
|
@@ -195,6 +196,7 @@ private async Task<ResolvedContainerBuildOptions> ResolveContainerBuildOptionsAs | |||||
| options.OutputPath = context.OutputPath; | ||||||
| options.ImageFormat = context.ImageFormat; | ||||||
| options.TargetPlatform = context.TargetPlatform; | ||||||
| options.Destination = context.Destination; | ||||||
| options.LocalImageName = context.LocalImageName ?? options.LocalImageName; | ||||||
| options.LocalImageTag = context.LocalImageTag ?? options.LocalImageTag; | ||||||
|
|
||||||
|
|
@@ -236,6 +238,22 @@ public async Task BuildImageAsync(IResource resource, CancellationToken cancella | |||||
|
|
||||||
| var options = await ResolveContainerBuildOptionsAsync(resource, cancellationToken).ConfigureAwait(false); | ||||||
|
|
||||||
| // Check if this resource needs a container runtime | ||||||
| if (await ResourcesRequireContainerRuntimeAsync([resource], cancellationToken).ConfigureAwait(false)) | ||||||
| { | ||||||
| logger.LogDebug("Checking {ContainerRuntimeName} health", ContainerRuntime.Name); | ||||||
|
|
||||||
| var containerRuntimeHealthy = await ContainerRuntime.CheckIfRunningAsync(cancellationToken).ConfigureAwait(false); | ||||||
|
|
||||||
| if (!containerRuntimeHealthy) | ||||||
| { | ||||||
| logger.LogError("Container runtime is not running or is unhealthy. Cannot build container image."); | ||||||
|
||||||
| logger.LogError("Container runtime is not running or is unhealthy. Cannot build container image."); | |
| logger.LogError("Container runtime '{ContainerRuntimeName}' is not running or is unhealthy. Cannot build container image.", ContainerRuntime.Name); |
Include the container runtime name here and in the exception below.
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.
Done in commit e25852e. Updated both the log error message and exception message to include the container runtime name. Also updated test assertions to work with the new format.
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.
The ResolveContainerBuildOptionsAsync method is being called twice in BuildImageAsync - once at line 239 and again inside ResourcesRequireContainerRuntimeAsync at line 531. This is inefficient since the options are already resolved and available. Consider passing the already-resolved options to ResourcesRequireContainerRuntimeAsync to avoid redundant callback invocations, or move the runtime check before the options resolution to avoid the duplicate work.