Closed HofmeisterAn closed 10 months ago
I implemented this feature, but after looking closer at the Java Properties files specification, it appears that inline comments are not supported. Java Properties files can only have single-line # comments
.
diff --git a/src/Testcontainers/Configurations/PropertiesFileConfiguration.cs b/src/Testcontainers/Configurations/PropertiesFileConfiguration.cs
index 6d0367c..3e9f716 100644
--- a/src/Testcontainers/Configurations/PropertiesFileConfiguration.cs
+++ b/src/Testcontainers/Configurations/PropertiesFileConfiguration.cs
@@ -4,6 +4,7 @@ namespace DotNet.Testcontainers.Configurations
using System.IO;
using System.Linq;
using System.Text.Json;
+ using System.Text.RegularExpressions;
using DotNet.Testcontainers.Images;
/// <summary>
@@ -11,6 +12,8 @@ namespace DotNet.Testcontainers.Configurations
/// </summary>
internal class PropertiesFileConfiguration : CustomConfiguration, ICustomConfiguration
{
+ private static readonly Regex CommentSeparator = new Regex("(?<!\\\\)[!#]", RegexOptions.None, TimeSpan.FromSeconds(1));
+
static PropertiesFileConfiguration()
{
}
@@ -42,11 +45,14 @@ namespace DotNet.Testcontainers.Configurations
: base(lines
.Select(line => line.Trim())
.Where(line => !string.IsNullOrEmpty(line))
- .Where(line => !line.StartsWith("#", StringComparison.Ordinal))
.Where(line => !line.StartsWith("!", StringComparison.Ordinal))
+ .Where(line => !line.StartsWith("#", StringComparison.Ordinal))
.Select(line => line.Split(new[] { '=', ':', ' ' }, 2, StringSplitOptions.RemoveEmptyEntries))
.Where(property => 2.Equals(property.Length))
- .ToDictionary(property => property[0], property => property[1]))
+ .ToDictionary(property => property[0], property => CommentSeparator.Split(property[1])[0]
+ .Replace("\\!", "!")
+ .Replace("\\#", "#")
+ .Trim()))
{
}
diff --git a/tests/Testcontainers.Tests/Unit/Configurations/CustomConfigurationTest.cs b/tests/Testcontainers.Tests/Unit/Configurations/CustomConfigurationTest.cs
index c30508a..0be5161 100644
--- a/tests/Testcontainers.Tests/Unit/Configurations/CustomConfigurationTest.cs
+++ b/tests/Testcontainers.Tests/Unit/Configurations/CustomConfigurationTest.cs
@@ -331,6 +331,20 @@ namespace DotNet.Testcontainers.Tests.Unit
ICustomConfiguration customConfiguration = new PropertiesFileConfiguration(new[] { configuration });
Assert.Equal(expected, customConfiguration.GetHubImageNamePrefix());
}
+
+ [Theory]
+ [InlineData("", null)]
+ [InlineData("!", null)]
+ [InlineData("#", null)]
+ [InlineData("docker.config=~/.docker/ ! The default configuration directory.", "~/.docker/")]
+ [InlineData("docker.config=~/.docker/ # The default configuration directory.", "~/.docker/")]
+ [InlineData("docker.config=~/.docker/\\!not_a_comment/", "~/.docker/!not_a_comment/")]
+ [InlineData("docker.config=~/.docker/\\#not_a_comment/", "~/.docker/#not_a_comment/")]
+ public void TrimCommentFromPropertiesFileProperty(string configuration, string expected)
+ {
+ ICustomConfiguration customConfiguration = new PropertiesFileConfiguration(new[] { configuration });
+ Assert.Equal(expected, customConfiguration.GetDockerConfig());
+ }
}
}
}
Problem
.NET does not have built-in support for
.properties
files. Testcontainers for .NET implements very simple parsing capabilities that do not support or follow many (or all) specs. For example, this is the case for inline comments, such as:Testcontainers will treat everything after the
=
as the value and is not able to connect to the Docker host. This is an unusual behavior, and something users usually won't expect.Solution
Instead of using everything after the
=
as the value, we can select everything until the comment character#
or!
appears. We may need to be careful here. AFAIK, the.properties
file spec supports escape sequences too, such as\#
and\!
.Benefit
Supporting inline comments in the future will prevent other users from running into an unnecessary issue when configuring Testcontainers.
Alternatives
Instead of using inline comments, users can put comments on a separate line.
Would you like to help contributing this enhancement?
Yes