Skip to content

Commit a825723

Browse files
lbussellCopilot
andauthored
Re-apply dry-run fix for CopyImageService publish config lookup (#1972)
PR #1966 fixed `CopyImageService.ImportImageAsync` to skip the `PublishConfiguration` lookup in dry-run mode. This fix was inadvertently reverted by the logging migration in #1968 due to a bad merge. 1. **Test** — Added `CopyImageServiceTests` with a test that exercises `ImportImageAsync` in dry-run mode with an empty `PublishConfiguration`, verifying it does not throw. 2. **Fix** — Moved `GetRegistryResource` calls and all Azure import logic after an early return for `isDryRun`, so dry-run mode only logs what would happen without requiring registry authentication details. Fixes pipeline failures in #1963. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ab5ca43 commit a825723

2 files changed

Lines changed: 64 additions & 26 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
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 System.Threading.Tasks;
5+
using Microsoft.DotNet.ImageBuilder.Configuration;
6+
using Microsoft.DotNet.ImageBuilder.Tests.Helpers;
7+
using Microsoft.Extensions.Logging;
8+
using Moq;
9+
using Shouldly;
10+
using Xunit;
11+
12+
namespace Microsoft.DotNet.ImageBuilder.Tests;
13+
14+
public class CopyImageServiceTests
15+
{
16+
/// <summary>
17+
/// When isDryRun is true and the publish configuration has no registry authentication,
18+
/// ImportImageAsync should succeed without throwing. This scenario occurs in PR validation
19+
/// pipelines where appsettings.json is not generated.
20+
/// </summary>
21+
[Fact]
22+
public async Task ImportImageAsync_DryRun_DoesNotRequirePublishConfiguration()
23+
{
24+
var emptyConfig = new PublishConfiguration();
25+
var service = new CopyImageService(
26+
Mock.Of<ILogger<CopyImageService>>(),
27+
Mock.Of<IAzureTokenCredentialProvider>(),
28+
ConfigurationHelper.CreateOptionsMock(emptyConfig));
29+
30+
await Should.NotThrowAsync(() =>
31+
service.ImportImageAsync(
32+
destTagNames: ["myacr.azurecr.io/repo:tag"],
33+
destAcrName: "myacr.azurecr.io",
34+
srcTagName: "repo:tag",
35+
srcRegistryName: "docker.io",
36+
isDryRun: true));
37+
}
38+
}

src/ImageBuilder/CopyImageService.cs

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,28 @@ public async Task ImportImageAsync(
5353
ContainerRegistryImportSourceCredentials? sourceCredentials = null,
5454
bool isDryRun = false)
5555
{
56+
Acr destAcr = Acr.Parse(destAcrName);
57+
58+
string action = isDryRun ? "(Dry run) Would have imported" : "Importing";
59+
string sourceImageName = DockerHelper.GetImageName(srcRegistryName, srcTagName);
60+
var destinationImageNames = destTagNames
61+
.Select(tag => $"'{DockerHelper.GetImageName(destAcr.Name, tag)}'")
62+
.ToList();
63+
string formattedDestinationImages = string.Join(", ", destinationImageNames);
64+
_logger.LogInformation("{Action} {DestinationImages} from '{SourceImage}'",
65+
action, formattedDestinationImages, sourceImageName);
66+
67+
if (isDryRun)
68+
{
69+
_logger.LogInformation("Importing skipped due to dry run.");
70+
return;
71+
}
72+
5673
var destResourceId = _publishConfig.GetRegistryResource(destAcrName);
5774
var srcResourceId = srcRegistryName is not null
5875
? _publishConfig.GetRegistryResource(srcRegistryName)
5976
: null;
6077

61-
Acr destAcr = Acr.Parse(destAcrName);
62-
6378
// Azure ACR import only supports one source identifier. Use ResourceId for ACR-to-ACR
6479
// imports (same tenant), or RegistryAddress for external registries.
6580
ContainerRegistryImportSource importSrc = new(srcTagName)
@@ -76,34 +91,19 @@ public async Task ImportImageAsync(
7691

7792
importImageContent.TargetTags.AddRange(destTagNames);
7893

79-
string action = isDryRun ? "(Dry run) Would have imported" : "Importing";
80-
string sourceImageName = DockerHelper.GetImageName(srcRegistryName, srcTagName);
81-
var destinationImageNames = destTagNames
82-
.Select(tag => $"'{DockerHelper.GetImageName(destAcr.Name, tag)}'")
83-
.ToList();
84-
string formattedDestinationImages = string.Join(", ", destinationImageNames);
85-
_logger.LogInformation($"{action} {formattedDestinationImages} from '{sourceImageName}'");
94+
ArmClient armClient = GetArmClientForAcr(destAcrName);
95+
ContainerRegistryResource registryResource = armClient.GetContainerRegistryResource(destResourceId);
8696

87-
if (!isDryRun)
97+
try
8898
{
89-
ArmClient armClient = GetArmClientForAcr(destAcrName);
90-
ContainerRegistryResource registryResource = armClient.GetContainerRegistryResource(destResourceId);
91-
92-
try
93-
{
94-
await RetryHelper.GetWaitAndRetryPolicy<Exception>(_logger)
95-
.ExecuteAsync(() => registryResource.ImportImageAsync(WaitUntil.Completed, importImageContent));
96-
}
97-
catch (Exception e)
98-
{
99-
_logger.LogError(e, "Importing Failure: {DestinationImages}", formattedDestinationImages);
100-
101-
throw;
102-
}
99+
await RetryHelper.GetWaitAndRetryPolicy<Exception>(_logger)
100+
.ExecuteAsync(() => registryResource.ImportImageAsync(WaitUntil.Completed, importImageContent));
103101
}
104-
else
102+
catch (Exception e)
105103
{
106-
_logger.LogInformation("Importing skipped due to dry run.");
104+
_logger.LogError(e, "Importing Failure: {DestinationImages}", formattedDestinationImages);
105+
106+
throw;
107107
}
108108
}
109109

0 commit comments

Comments
 (0)