Skip to content

Commit 72d0399

Browse files
Copilotdavidfowlmitchdenny
authored
Follow-up changes to PR #11852: Preserve ContainerImageAnnotation and encapsulate dockerfile factory logic (#11870)
* Initial plan * Implement changes to preserve ContainerImageAnnotation and encapsulate dockerfile factory logic Co-authored-by: davidfowl <[email protected]> * Update WithImage and WithImageTag to properly handle DockerfileBuildAnnotation Co-authored-by: davidfowl <[email protected]> * Add TryGetContainerImageName overload with useBuiltImage parameter Co-authored-by: mitchdenny <[email protected]> * Use container image name logic inside AzureDeployingContext and ResourceCOntainerImageBuilder. * Fix tests. * Fix failing compose test. * Refactor sync WithDockerfile callback to call async version for code reuse Co-authored-by: mitchdenny <[email protected]> * Disable failing test. * Disable flaky test. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: davidfowl <[email protected]> Co-authored-by: mitchdenny <[email protected]> Co-authored-by: Mitch Denny <[email protected]>
1 parent 9c6557f commit 72d0399

File tree

10 files changed

+168
-67
lines changed

10 files changed

+168
-67
lines changed

‎src/Aspire.Hosting.Azure/AzureDeployingContext.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,12 @@ private async Task PushImagesToAllRegistries(Dictionary<IContainerRegistry, List
430430
.Where(r => r.RequiresImageBuildAndPush())
431431
.Select(async resource =>
432432
{
433-
var localImageName = resource.Name.ToLowerInvariant();
433+
if (!resource.TryGetContainerImageName(out var localImageName))
434+
{
435+
// For resources without an explicit image name, we use the resource name.
436+
localImageName = resource.Name.ToLowerInvariant();
437+
}
438+
434439
IValueProvider cir = new ContainerImageReference(resource);
435440
var targetTag = await cir.GetValueAsync(cancellationToken).ConfigureAwait(false);
436441

‎src/Aspire.Hosting/ApplicationModel/DockerfileBuildAnnotation.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,16 @@ public class DockerfileBuildAnnotation(string contextPath, string dockerfilePath
4242
/// and the content will be written to a generated file path.
4343
/// </summary>
4444
public Func<DockerfileFactoryContext, Task<string>>? DockerfileFactory { get; init; }
45+
46+
/// <summary>
47+
/// Gets or sets the image name for the generated container image.
48+
/// When set, this will be used as the container image name instead of the value from ContainerImageAnnotation.
49+
/// </summary>
50+
public string? ImageName { get; set; }
51+
52+
/// <summary>
53+
/// Gets or sets the image tag for the generated container image.
54+
/// When set, this will be used as the container image tag instead of the value from ContainerImageAnnotation.
55+
/// </summary>
56+
public string? ImageTag { get; set; }
4557
}

‎src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -538,6 +538,29 @@ public static EndpointReference GetEndpoint(this IResourceWithEndpoints resource
538538
/// <returns>True if the container image name was found, otherwise false.</returns>
539539
public static bool TryGetContainerImageName(this IResource resource, [NotNullWhen(true)] out string? imageName)
540540
{
541+
return TryGetContainerImageName(resource, useBuiltImage: true, out imageName);
542+
}
543+
544+
/// <summary>
545+
/// Attempts to get the container image name from the given resource.
546+
/// </summary>
547+
/// <param name="resource">The resource to get the container image name from.</param>
548+
/// <param name="useBuiltImage">When true, uses the image name from DockerfileBuildAnnotation if present. When false, uses only ContainerImageAnnotation.</param>
549+
/// <param name="imageName">The container image name if found, otherwise null.</param>
550+
/// <returns>True if the container image name was found, otherwise false.</returns>
551+
public static bool TryGetContainerImageName(this IResource resource, bool useBuiltImage, [NotNullWhen(true)] out string? imageName)
552+
{
553+
// First check if there's a DockerfileBuildAnnotation with an image name/tag
554+
// This takes precedence over the ContainerImageAnnotation when building from a Dockerfile
555+
if (useBuiltImage &&
556+
resource.Annotations.OfType<DockerfileBuildAnnotation>().SingleOrDefault() is { } buildAnnotation &&
557+
!string.IsNullOrEmpty(buildAnnotation.ImageName))
558+
{
559+
var tagSuffix = string.IsNullOrEmpty(buildAnnotation.ImageTag) ? string.Empty : $":{buildAnnotation.ImageTag}";
560+
imageName = $"{buildAnnotation.ImageName}{tagSuffix}";
561+
return true;
562+
}
563+
541564
if (resource.Annotations.OfType<ContainerImageAnnotation>().LastOrDefault() is { } imageAnnotation)
542565
{
543566
var registryPrefix = string.IsNullOrEmpty(imageAnnotation.Registry) ? string.Empty : $"{imageAnnotation.Registry}/";

‎src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,15 @@ public static IResourceBuilder<T> WithImageTag<T>(this IResourceBuilder<T> build
215215
if (builder.Resource.Annotations.OfType<ContainerImageAnnotation>().LastOrDefault() is { } existingImageAnnotation)
216216
{
217217
existingImageAnnotation.Tag = tag;
218+
219+
// If there's a DockerfileBuildAnnotation with an image tag, update it as well
220+
// so that the user's explicit tag preference is respected
221+
if (builder.Resource.Annotations.OfType<DockerfileBuildAnnotation>().SingleOrDefault() is { } buildAnnotation &&
222+
!string.IsNullOrEmpty(buildAnnotation.ImageTag))
223+
{
224+
buildAnnotation.ImageTag = tag;
225+
}
226+
218227
return builder;
219228
}
220229

@@ -297,6 +306,14 @@ public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder,
297306
imageAnnotation.Tag = parsedReference.Tag ?? tag ?? "latest";
298307
}
299308

309+
// If there's a DockerfileBuildAnnotation with an image name/tag, clear them
310+
// so that the user's explicit image preference is respected
311+
if (builder.Resource.Annotations.OfType<DockerfileBuildAnnotation>().SingleOrDefault() is { } buildAnnotation)
312+
{
313+
buildAnnotation.ImageName = null;
314+
buildAnnotation.ImageTag = null;
315+
}
316+
300317
return builder;
301318
}
302319

@@ -489,6 +506,15 @@ public static IResourceBuilder<T> WithDockerfile<T>(this IResourceBuilder<T> bui
489506
var imageTag = ImageNameGenerator.GenerateImageTag(builder);
490507
var annotation = new DockerfileBuildAnnotation(fullyQualifiedContextPath, fullyQualifiedDockerfilePath, stage);
491508

509+
// If there's already a ContainerImageAnnotation, don't overwrite it.
510+
// Instead, store the generated image name and tag on the DockerfileBuildAnnotation.
511+
if (builder.Resource.Annotations.OfType<ContainerImageAnnotation>().LastOrDefault() is { })
512+
{
513+
annotation.ImageName = imageName;
514+
annotation.ImageTag = imageTag;
515+
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace);
516+
}
517+
492518
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace)
493519
.WithImageRegistry(registry: null)
494520
.WithImage(imageName)
@@ -535,26 +561,9 @@ public static IResourceBuilder<T> WithDockerfile<T>(this IResourceBuilder<T> bui
535561
public static IResourceBuilder<T> WithDockerfile<T>(this IResourceBuilder<T> builder, string contextPath, Func<DockerfileFactoryContext, string> dockerfileFactory, string? stage = null) where T : ContainerResource
536562
{
537563
ArgumentNullException.ThrowIfNull(builder);
538-
ArgumentException.ThrowIfNullOrEmpty(contextPath);
539564
ArgumentNullException.ThrowIfNull(dockerfileFactory);
540565

541-
var fullyQualifiedContextPath = Path.GetFullPath(contextPath, builder.ApplicationBuilder.AppHostDirectory)
542-
.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
543-
544-
// Create a unique temporary Dockerfile path for this resource
545-
var tempDockerfilePath = Path.Combine(Path.GetTempPath(), $"Dockerfile.{builder.Resource.Name}.{Guid.NewGuid():N}");
546-
547-
var imageName = ImageNameGenerator.GenerateImageName(builder);
548-
var imageTag = ImageNameGenerator.GenerateImageTag(builder);
549-
var annotation = new DockerfileBuildAnnotation(fullyQualifiedContextPath, tempDockerfilePath, stage)
550-
{
551-
DockerfileFactory = context => Task.FromResult(dockerfileFactory(context))
552-
};
553-
554-
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace)
555-
.WithImageRegistry(registry: null)
556-
.WithImage(imageName)
557-
.WithImageTag(imageTag);
566+
return builder.WithDockerfile(contextPath, context => Task.FromResult(dockerfileFactory(context)), stage);
558567
}
559568

560569
/// <summary>
@@ -614,6 +623,15 @@ public static IResourceBuilder<T> WithDockerfile<T>(this IResourceBuilder<T> bui
614623
DockerfileFactory = dockerfileFactory
615624
};
616625

626+
// If there's already a ContainerImageAnnotation, don't overwrite it.
627+
// Instead, store the generated image name and tag on the DockerfileBuildAnnotation.
628+
if (builder.Resource.Annotations.OfType<ContainerImageAnnotation>().LastOrDefault() is { })
629+
{
630+
annotation.ImageName = imageName;
631+
annotation.ImageTag = imageTag;
632+
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace);
633+
}
634+
617635
return builder.WithAnnotation(annotation, ResourceAnnotationMutationBehavior.Replace)
618636
.WithImageRegistry(registry: null)
619637
.WithImage(imageName)

‎src/Aspire.Hosting/Dcp/DcpExecutor.cs‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,17 +1516,7 @@ private static async Task ApplyBuildArgumentsAsync(Container dcpContainerResourc
15161516
if (modelContainerResource.Annotations.OfType<DockerfileBuildAnnotation>().SingleOrDefault() is { } dockerfileBuildAnnotation)
15171517
{
15181518
// If there's a factory, generate the Dockerfile content and write it to the specified path
1519-
if (dockerfileBuildAnnotation.DockerfileFactory is not null)
1520-
{
1521-
var context = new DockerfileFactoryContext
1522-
{
1523-
Services = serviceProvider,
1524-
Resource = modelContainerResource,
1525-
CancellationToken = cancellationToken
1526-
};
1527-
var dockerfileContent = await dockerfileBuildAnnotation.DockerfileFactory(context).ConfigureAwait(false);
1528-
await File.WriteAllTextAsync(dockerfileBuildAnnotation.DockerfilePath, dockerfileContent, cancellationToken).ConfigureAwait(false);
1529-
}
1519+
await DockerfileHelper.ExecuteDockerfileFactoryAsync(dockerfileBuildAnnotation, modelContainerResource, serviceProvider, cancellationToken).ConfigureAwait(false);
15301520

15311521
var dcpBuildArgs = new List<EnvVar>();
15321522

‎src/Aspire.Hosting/Publishing/ManifestPublishingContext.cs‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -306,19 +306,10 @@ private async Task WriteBuildContextAsync(ContainerResource container)
306306
string dockerfilePath = annotation.DockerfilePath;
307307

308308
// If there's a factory, generate the Dockerfile content and write it to both the original path and a resource-specific path
309+
await DockerfileHelper.ExecuteDockerfileFactoryAsync(annotation, container, ExecutionContext.ServiceProvider, CancellationToken).ConfigureAwait(false);
310+
309311
if (annotation.DockerfileFactory is not null)
310312
{
311-
var context = new DockerfileFactoryContext
312-
{
313-
Services = ExecutionContext.ServiceProvider,
314-
Resource = container,
315-
CancellationToken = CancellationToken
316-
};
317-
var dockerfileContent = await annotation.DockerfileFactory(context).ConfigureAwait(false);
318-
319-
// Always write to the original DockerfilePath so code looking at that path still works
320-
await File.WriteAllTextAsync(annotation.DockerfilePath, dockerfileContent, CancellationToken).ConfigureAwait(false);
321-
322313
// Copy to a resource-specific path in the manifest output directory for publishing
323314
var manifestDirectory = Path.GetDirectoryName(ManifestPath)!;
324315
var resourceDockerfilePath = Path.Combine(manifestDirectory, $"{container.Name}.Dockerfile");

‎src/Aspire.Hosting/Publishing/ResourceContainerImageBuilder.cs‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -208,20 +208,28 @@ await BuildProjectContainerImageAsync(
208208
cancellationToken).ConfigureAwait(false);
209209
return;
210210
}
211-
else if (resource.TryGetLastAnnotation<ContainerImageAnnotation>(out var containerImageAnnotation))
211+
else if (resource.TryGetLastAnnotation<DockerfileBuildAnnotation>(out var dockerfileBuildAnnotation))
212212
{
213-
if (resource.TryGetLastAnnotation<DockerfileBuildAnnotation>(out var dockerfileBuildAnnotation))
213+
if (!resource.TryGetContainerImageName(out var imageName))
214214
{
215-
// This is a container resource so we'll use the container runtime to build the image
216-
await BuildContainerImageFromDockerfileAsync(
217-
resource,
218-
dockerfileBuildAnnotation,
219-
containerImageAnnotation.Image,
220-
step,
221-
options,
222-
cancellationToken).ConfigureAwait(false);
223-
return;
215+
throw new InvalidOperationException("Resource image name could not be determined.");
224216
}
217+
218+
// This is a container resource so we'll use the container runtime to build the image
219+
await BuildContainerImageFromDockerfileAsync(
220+
resource,
221+
dockerfileBuildAnnotation,
222+
imageName,
223+
step,
224+
options,
225+
cancellationToken).ConfigureAwait(false);
226+
return;
227+
}
228+
else if (resource.TryGetLastAnnotation<ContainerImageAnnotation>(out var _))
229+
{
230+
// This resource already has a container image associated with it so no build is needed.
231+
logger.LogInformation("Resource {ResourceName} already has a container image associated and no build annotation. Skipping build.", resource.Name);
232+
return;
225233
}
226234
else
227235
{

‎src/Aspire.Hosting/Utils/DockerfileHelper.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using Aspire.Hosting.ApplicationModel;
5+
6+
namespace Aspire.Hosting.Utils;
7+
8+
internal static class DockerfileHelper
9+
{
10+
/// <summary>
11+
/// Executes the dockerfile factory if present and writes the generated content to the specified path.
12+
/// </summary>
13+
/// <param name="annotation">The dockerfile build annotation containing the factory.</param>
14+
/// <param name="resource">The resource for which the dockerfile is being generated.</param>
15+
/// <param name="serviceProvider">The service provider to be passed to the factory context.</param>
16+
/// <param name="cancellationToken">The cancellation token.</param>
17+
/// <returns>A task representing the asynchronous operation.</returns>
18+
public static async Task ExecuteDockerfileFactoryAsync(
19+
DockerfileBuildAnnotation annotation,
20+
IResource resource,
21+
IServiceProvider serviceProvider,
22+
CancellationToken cancellationToken)
23+
{
24+
if (annotation.DockerfileFactory is not null)
25+
{
26+
var context = new DockerfileFactoryContext
27+
{
28+
Services = serviceProvider,
29+
Resource = resource,
30+
CancellationToken = cancellationToken
31+
};
32+
var dockerfileContent = await annotation.DockerfileFactory(context).ConfigureAwait(false);
33+
await File.WriteAllTextAsync(annotation.DockerfilePath, dockerfileContent, cancellationToken).ConfigureAwait(false);
34+
}
35+
}
36+
}

‎tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ public async Task DeployAsync_WithDockerfile_Works()
254254

255255
// Verify specific tag call was made (local "api" to target in testregistry with deployment tag)
256256
Assert.Contains(mockImageBuilder.TagImageCalls, call =>
257-
call.localImageName == "api" &&
257+
call.localImageName.StartsWith("api:") &&
258258
call.targetImageName.StartsWith("testregistry.azurecr.io/") &&
259259
call.targetImageName.Contains("aspire-deploy-"));
260260

@@ -322,7 +322,7 @@ public async Task DeployAsync_WithProjectResource_Works()
322322
}
323323

324324
[Fact]
325-
[ActiveIssue("https://github.com/dotnet/aspire/issues/11728", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))]
325+
[ActiveIssue("https://github.com/dotnet/aspire/issues/11728")]
326326
public async Task DeployAsync_WithMultipleComputeEnvironments_Works()
327327
{
328328
// Arrange
@@ -405,7 +405,7 @@ public async Task DeployAsync_WithMultipleComputeEnvironments_Works()
405405
call.targetImageName.StartsWith("aasregistry.azurecr.io/") &&
406406
call.targetImageName.Contains("aspire-deploy-"));
407407
Assert.Contains(mockImageBuilder.TagImageCalls, call =>
408-
call.localImageName == "python-app" &&
408+
call.localImageName.StartsWith("python-app:") &&
409409
call.targetImageName.StartsWith("acaregistry.azurecr.io/") &&
410410
call.targetImageName.Contains("aspire-deploy-"));
411411

@@ -746,7 +746,7 @@ public async Task DeployAsync_WithParametersInArguments_DiscoversAndPromptsForPa
746746
}
747747

748748
[Fact]
749-
[ActiveIssue("https://github.com/dotnet/aspire/issues/11728", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))]
749+
[ActiveIssue("https://github.com/dotnet/aspire/issues/11728")]
750750
public async Task DeployAsync_WithAzureFunctionsProject_Works()
751751
{
752752
// Arrange

0 commit comments

Comments
 (0)