[release/8.0.1xx] Fix the parsing of ContainerImageTags to report invalid tags instead of silently swallowing them. (#38045)

Co-authored-by: Chet Husk <[email protected]>

Author: github-actions[bot]

src/Containers/Microsoft.NET.Build.Containers/Tasks/ParseContainerProperties.cs

@@ -100,12 +100,12 @@ public override bool Execute()
                 Log.LogErrorWithCodeFromResources(nameof(Strings.InvalidTag), nameof(ContainerImageTag), ContainerImageTag);
             }
         }
-        else if (ContainerImageTags.Length != 0 && TryValidateTags(ContainerImageTags, out var valids, out var invalids))
+        else if (ContainerImageTags.Length != 0)
         {
-            validTags = valids;
-            if (invalids.Any())
+            (validTags, var invalidTags) = TryValidateTags(ContainerImageTags);
+            if (invalidTags.Any())
             {
-                Log.LogErrorWithCodeFromResources(nameof(Strings.InvalidTags), nameof(ContainerImageTags), String.Join(",", invalids));
+                Log.LogErrorWithCodeFromResources(nameof(Strings.InvalidTags), nameof(ContainerImageTags), String.Join(",", invalidTags));
                 return !Log.HasLoggedErrors;
             }
         }
@@ -200,7 +200,7 @@ private void ValidateEnvironmentVariables()
         }
     }
 
-    private static bool TryValidateTags(string[] inputTags, out string[] validTags, out string[] invalidTags)
+    private static (string[] validTags, string[] invalidTags) TryValidateTags(string[] inputTags)
     {
         var v = new List<string>();
         var i = new List<string>();
@@ -215,8 +215,6 @@ private static bool TryValidateTags(string[] inputTags, out string[] validTags,
                 i.Add(tag);
             }
         }
-        validTags = v.ToArray();
-        invalidTags = i.ToArray();
-        return invalidTags.Length == 0;
+        return (v.ToArray(), i.ToArray());
     }
 }

src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/MSBuildCollection.cs

@@ -0,0 +1,22 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.NET.Build.Containers.IntegrationTests;
+
+using Xunit;
+
+/// <summary>
+/// Collection definition for tests that require MSBuild to be run.
+/// </summary>
+/// <remarks>
+/// This collection is used to ensure that tests that require MSBuild are run serially.
+/// The MSBuild engine only allows a single logical Build to run at once, so running tests
+/// that require MSBuild in parallel can cause tests to fail./
+/// </remarks>
+[CollectionDefinition(nameof(MSBuildCollection), DisableParallelization = true)]
+public class MSBuildCollection : ICollectionFixture<MSBuildCollection>
+{
+    // This class has no code, and is never created. Its purpose is simply
+    // to be the place to apply [CollectionDefinition] and all the
+    // ICollectionFixture<> interfaces.
+}

src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/ParseContainerPropertiesTests.cs

@@ -8,21 +8,22 @@
 
 namespace Microsoft.NET.Build.Containers.Tasks.IntegrationTests;
 
-[Collection("Docker tests")]
+[Collection(nameof(MSBuildCollection))]
 public class ParseContainerPropertiesTests
 {
-    [DockerAvailableFact]
+    [Fact]
     public void Baseline()
     {
-        var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerRepository] = "dotnet/testimage",
             [ContainerImageTags] = "7.0;latest"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.True(instance.Build(new[]{ComputeContainerConfig}, new [] { logs }, null, out var outputs));
+        Assert.True(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
         Assert.Equal("mcr.microsoft.com", instance.GetPropertyValue(ContainerBaseRegistry));
         Assert.Equal("dotnet/runtime", instance.GetPropertyValue(ContainerBaseName));
@@ -33,58 +34,62 @@ public void Baseline()
         instance.GetItems("ProjectCapability").Select(i => i.EvaluatedInclude).ToArray().Should().BeEquivalentTo(new[] { "NetSdkOCIImageBuild" });
     }
 
-    [DockerAvailableFact]
+    [Fact]
     public void SpacesGetReplacedWithDashes()
     {
-         var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet runtime:7.0",
             [ContainerRegistry] = "localhost:5010"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.True(instance.Build(new[]{ComputeContainerConfig}, new [] { logs }, null, out var outputs));
+        Assert.True(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
-        Assert.Equal("mcr.microsoft.com",instance.GetPropertyValue(ContainerBaseRegistry));
+        Assert.Equal("mcr.microsoft.com", instance.GetPropertyValue(ContainerBaseRegistry));
         Assert.Equal("dotnet-runtime", instance.GetPropertyValue(ContainerBaseName));
         Assert.Equal("7.0", instance.GetPropertyValue(ContainerBaseTag));
     }
 
-    [DockerAvailableFact]
+    [Fact]
     public void RegexCatchesInvalidContainerNames()
     {
-         var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerRepository] = "dotnet testimage",
             [ContainerImageTag] = "5.0"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.True(instance.Build(new[]{ComputeContainerConfig}, new [] { logs }, null, out var outputs));
+        Assert.True(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
         Assert.Contains(logs.Messages, m => m.Message?.Contains("'dotnet testimage' was not a valid container image name, it was normalized to 'dotnet-testimage'") == true);
     }
 
-    [DockerAvailableFact]
+    [Fact]
     public void RegexCatchesInvalidContainerTags()
     {
-        var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerRepository] = "dotnet/testimage",
             [ContainerImageTag] = "5 0"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.False(instance.Build(new[]{ComputeContainerConfig},  new [] { logs }, null, out var outputs));
+        Assert.False(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
         Assert.True(logs.Errors.Count > 0);
         Assert.Equal(logs.Errors[0].Code, ErrorCodes.CONTAINER2007);
     }
 
-    [DockerAvailableFact]
+    [Fact]
     public void CanOnlySupplyOneOfTagAndTags()
     {
-        var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerRepository] = "dotnet/testimage",
@@ -93,41 +98,60 @@ public void CanOnlySupplyOneOfTagAndTags()
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.False(instance.Build(new[]{ComputeContainerConfig},  new [] { logs }, null, out var outputs));
+        Assert.False(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
         Assert.True(logs.Errors.Count > 0);
         Assert.Equal(logs.Errors[0].Code, ErrorCodes.CONTAINER2008);
     }
 
-    [DockerAvailableFact]
+    [Fact]
+    public void InvalidTagsThrowError()
+    {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
+            [ContainerBaseImage] = "mcr.microsoft.com/dotnet/aspnet:8.0",
+            [ContainerRepository] = "dotnet/testimage",
+            [ContainerImageTags] = "'latest;oldest'"
+        });
+        using var _ = d;
+        var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
+        Assert.False(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
+
+        Assert.True(logs.Errors.Count > 0);
+        Assert.Equal(logs.Errors[0].Code, ErrorCodes.CONTAINER2010);
+    }
+
+    [Fact]
     public void FailsOnCompletelyInvalidRepositoryNames()
     {
-        var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerImageName] = "㓳㓴㓵㓶㓷㓹㓺㓻",
             [ContainerImageTag] = "5.0"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.False(instance.Build(new[]{ComputeContainerConfig},  new [] { logs }, null, out var outputs));
+        Assert.False(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
         Assert.True(logs.Errors.Count > 0);
         Assert.Equal(logs.Errors[0].Code, ErrorCodes.CONTAINER2005);
     }
 
-    [DockerAvailableFact]
+    [Fact]
     public void FailsWhenFirstCharIsAUnicodeLetterButNonLatin()
     {
-        var (project, logs, d) = ProjectInitializer.InitProject(new () {
+        var (project, logs, d) = ProjectInitializer.InitProject(new()
+        {
             [ContainerBaseImage] = "mcr.microsoft.com/dotnet/runtime:7.0",
             [ContainerRegistry] = "localhost:5010",
             [ContainerImageName] = "㓳but-otherwise-valid",
             [ContainerImageTag] = "5.0"
         });
         using var _ = d;
         var instance = project.CreateProjectInstance(global::Microsoft.Build.Execution.ProjectInstanceSettings.None);
-        Assert.False(instance.Build(new[]{ComputeContainerConfig},  new [] { logs }, null, out var outputs));
+        Assert.False(instance.Build(new[] { ComputeContainerConfig }, new[] { logs }, null, out var outputs));
 
         Assert.True(logs.Errors.Count > 0);
         Assert.Equal(logs.Errors[0].Code, ErrorCodes.CONTAINER2005);

src/Tests/Microsoft.NET.Build.Containers.IntegrationTests/TargetsTests.cs

@@ -7,6 +7,7 @@
 
 namespace Microsoft.NET.Build.Containers.Targets.IntegrationTests;
 
+[Collection(nameof(MSBuildCollection))]
 public class TargetsTests
 {
     [InlineData("SelfContained", true, "/app/foo.exe")]