From f6f5c4e79bf4dd0cc894352ea9ffdb6f52fb3447 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Thu, 17 Sep 2026 17:11:50 +1000 Subject: [PATCH] Preserve line endings and trailing newline when updating image tags (SF-3581) (#2162) Co-authored-by: Claude Opus 5 (1M context) --- .../Plumbing/Extensions/StringExtensions.cs | 5 + .../ArgoCD/Helm/HelmValuesEditorTests.cs | 45 +++-- ...elmValuesImageReplaceStepVariablesTests.cs | 99 ++++++++-- .../ArgoCD/Helm/HelmYamlParserTests.cs | 174 ++++++++++++------ .../ArgoCD/YamlStreamLoaderTests.cs | 56 ++++++ .../Fixtures/Util/StringExtensionsFixture.cs | 14 ++ .../InlineStrategicMergeImageReplacer.cs | 5 +- source/Calamari/ArgoCD/Helm/HelmYamlParser.cs | 8 +- .../ArgoCD/InlineJsonPatchReplacer.cs | 4 +- .../ArgoCD/YamlJson6902PatchImageReplacer.cs | 11 +- source/Calamari/ArgoCD/YamlStreamLoader.cs | 34 ++-- 11 files changed, 329 insertions(+), 126 deletions(-) create mode 100644 source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs diff --git a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs index 4ea21405bc..8d2dbb9739 100644 --- a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs +++ b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs @@ -99,6 +99,11 @@ public static string ToCamelCase(this string text) : null; } + public static bool HasTrailingNewLine(this string? input) + { + return input != null && (input.EndsWith("\n") || input.EndsWith("\r")); + } + public static string EnsureDoubleQuoteIfContainsSpaces(this string text) => EnsureDoubleQuote(text, t => t.Contains(" ")); public static string EnsureDoubleQuote(this string text) => EnsureDoubleQuote(text, t => !t.EndsWith("\"") && !t.StartsWith("\"")); public static string EnsureDoubleQuote(this string text, Predicate shouldQuote) => shouldQuote(text) ? $"\"{text}\"" : text; diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs index de6d2736c0..847cce44ee 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs @@ -98,29 +98,34 @@ public void GenerateVariableDictionary_ReturnsDictionaryOfNodeValuesWithValues() result.Should().BeEquivalentTo(expected); } - [Test] - public void UpdateNodeValue_ReturnsModifiedYaml() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_ReturnsModifiedYaml(string newLine) { - const string yamlContent = @"root: - node1: ""node1value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; + var yamlContent = """ + root: + node1: "node1value" + node2: + node2Nest: + node2nestedValue: "banana" + node2Child1: "node2child1value" + node2Child2: 42 + + """.ReplaceLineEndings(newLine); + var result = HelmValuesEditor.UpdateNodeValue(yamlContent, "root.node1", "awesome new value"); - const string expected = @"root: - node1: ""awesome new value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expected.ReplaceLineEndings()); + var expected = """ + root: + node1: "awesome new value" + node2: + node2Nest: + node2nestedValue: "banana" + node2Child1: "node2child1value" + node2Child2: 42 + + """.ReplaceLineEndings(newLine); + result.Should().Be(expected); } } } diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs index e68151e7b3..9089edf749 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs @@ -1,5 +1,4 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; @@ -122,13 +121,15 @@ public void StructuredValue_ImageOnNonDefaultRegistry_UpdatesFullRefAndTracksWit [Test] public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() { - const string yaml = @" -nginx: - tag: 1.0 -redis: - tag: 1.0 -"; - + const string yaml = """ + + nginx: + tag: 1.0 + redis: + tag: 1.0 + + """; + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); var images = new List { @@ -137,10 +138,86 @@ public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() var result = replacer.UpdateImages(images); + const string expectedYaml = """ + + nginx: + tag: 1.27.1 + redis: + tag: 1.0 + + """; + using var scope = new AssertionScope(); result.UpdatedImageReferences.Should().BeEquivalentTo(["nginx:1.27.1"]); - result.UpdatedContents.Should().Contain($"nginx:{Environment.NewLine} tag: 1.27.1"); - result.UpdatedContents.Should().Contain($"redis:{Environment.NewLine} tag: 1.0"); + result.UpdatedContents.ReplaceLineEndings("\n").Should().Be(expectedYaml.ReplaceLineEndings("\n")); + } + + // The endings are forced with ReplaceLineEndings rather than inherited from the literal: + // .gitattributes checks .cs files out with native endings, so an inherited ending always matches + // Environment.NewLine and the assertion cannot distinguish "preserved the input's endings" from + // "used the agent's". The customer's case was an LF file on a Windows agent. + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdatedYaml_PreservesTheInputLineEndings(string newLine) + { + var yaml = """ + + nginx: + tag: 1.0 + redis: + tag: 1.0 + + """.ReplaceLineEndings(newLine); + + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); + var images = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "nginx.tag") + }; + + var result = replacer.UpdateImages(images); + + var expectedYaml = """ + + nginx: + tag: 1.27.1 + redis: + tag: 1.0 + + """.ReplaceLineEndings(newLine); + + result.UpdatedContents.Should().Be(expectedYaml); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdatedYaml_WithoutATrailingNewline_DoesNotAddOne(string newLine) + { + var yaml = """ + + nginx: + tag: 1.0 + redis: + tag: 1.0 + """.ReplaceLineEndings(newLine); + + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); + var images = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "nginx.tag") + }; + + var result = replacer.UpdateImages(images); + + var expectedYaml = """ + + nginx: + tag: 1.27.1 + redis: + tag: 1.0 + """.ReplaceLineEndings(newLine); + + result.UpdatedContents.Should().Be(expectedYaml); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs index fff879ad7d..c26b92895a 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using Calamari.ArgoCD.Helm; using FluentAssertions; using NUnit.Framework; @@ -51,96 +51,156 @@ public void GetValueAtPath_ReturnsTheValueOfTheSpecifiedNode(string path, string result.Should().Be(expected); } - [Test] - public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument(string newLine) { - const string yamlContent = @" -root: - node1: 42 - node2: stable -"; + var yamlContent = """ + + root: + node1: 42 + node2: stable + + """.ReplaceLineEndings(newLine); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - node2: stable -"; + var result = sut.UpdateContentForPath("root.node1", "69"); + + var expected = """ + + root: + node1: 69 + node2: stable + + """.ReplaceLineEndings(newLine); + result.Should().Be(expected); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) + { + var yamlContent = """ + + root: + node1: 42 + node2: "latest" + + """.ReplaceLineEndings(newLine); + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node2", "stable"); + + var expected = """ + + root: + node1: 42 + node2: "stable" + + """.ReplaceLineEndings(newLine); + result.Should().Be(expected); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) + { + var yamlContent = """ + + root: + node1: 42 + node2: 'latest' + + """.ReplaceLineEndings(newLine); + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node2", "stable"); + + var expected = """ + + root: + node1: 42 + node2: 'stable' + + """.ReplaceLineEndings(newLine); + result.Should().Be(expected); + } + + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput(string newLine) + { + var yamlContent = "\nroot:\n node1: 42\n \n".ReplaceLineEndings(newLine); + + var sut = new HelmYamlParser(yamlContent); var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + var expected = "\nroot:\n node1: 69\n \n".ReplaceLineEndings(newLine); + result.Should().Be(expected); } [Test] - public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + public void UpdateNodeValue_WithCrlfLineEndings_PreservesCrlfOnEveryLine() { - const string yamlContent = @" -root: - node1: 42 - node2: ""latest"" -"; + const string yamlContent = "root:\r\n node1: 42\r\n node2: stable\r\n"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: ""stable"" -"; + var result = sut.UpdateContentForPath("root.node1", "69"); - var result = sut.UpdateContentForPath("root.node2", "stable"); + result.Should().Be("root:\r\n node1: 69\r\n node2: stable\r\n"); + } + + [Test] + public void UpdateNodeValue_WithLfLineEndings_PreservesLfOnEveryLine() + { + const string yamlContent = "root:\n node1: 42\n node2: stable\n"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be("root:\n node1: 69\n node2: stable\n"); } [Test] - public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + public void UpdateNodeValue_WithNoTrailingNewline_DoesNotAddOne() { - const string yamlContent = @" -root: - node1: 42 - node2: 'latest' -"; + const string yamlContent = "root:\n node1: 42"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: 'stable' -"; + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be("root:\n node1: 69"); + } + + [Test] + public void UpdateNodeValue_WithCrlfAndNoTrailingNewline_PreservesBoth() + { + const string yamlContent = "root:\r\n node1: 42\r\n node2: \"latest\""; + + var sut = new HelmYamlParser(yamlContent); var result = sut.UpdateContentForPath("root.node2", "stable"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be("root:\r\n node1: 42\r\n node2: \"stable\""); } [Test] - public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput() + public void UpdateNodeValue_WithUnchangedPath_ReturnsContentByteForByte() { - const string yamlContent = @" -root: - node1: 42 - -"; + const string yamlContent = "root:\r\n node1: 42\r\n"; var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - -"; - - var result = sut.UpdateContentForPath("root.node1", "69"); + var result = sut.UpdateContentForPath("root.missing", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(yamlContent); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs b/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs new file mode 100644 index 0000000000..0937cac8c4 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs @@ -0,0 +1,56 @@ +using Calamari.ArgoCD; +using FluentAssertions; +using NUnit.Framework; + +namespace Calamari.Tests.ArgoCD +{ + [TestFixture] + public class YamlStreamLoaderTests + { + [Test] + public void SerializeDocuments_WithCrlfOriginal_EmitsCrlfOnEveryLine() + { + const string original = "kind: Kustomization\r\nnamespace: dev\r\n"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\r\nnamespace: dev\r\n"); + } + + [Test] + public void SerializeDocuments_WithLfOriginal_EmitsLfOnEveryLine() + { + const string original = "kind: Kustomization\nnamespace: dev\n"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\nnamespace: dev\n"); + } + + [Test] + public void SerializeDocuments_WithOriginalMissingTrailingNewline_DoesNotAddOne() + { + const string original = "kind: Kustomization\nnamespace: dev"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\nnamespace: dev"); + } + + [Test] + public void SerializeDocuments_WithMultipleDocuments_SeparatesUsingTheOriginalLineEnding() + { + const string original = "kind: First\r\n---\r\nkind: Second\r\n"; + + var result = Serialize(original); + + result.Should().Be("kind: First\r\n---\r\nkind: Second\r\n"); + } + + static string Serialize(string original) + { + var stream = YamlStreamLoader.TryLoadSilent(original); + return YamlStreamLoader.SerializeDocuments(stream!.Documents, original); + } + } +} diff --git a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs index ef72f9a9c1..9c48a6b7a4 100644 --- a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs +++ b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs @@ -44,6 +44,20 @@ public void AsRelativePathFrom(string source, string baseDirectory, string expec Assert.AreEqual(expected, source.AsRelativePathFrom(baseDirectory)); } + [TestCase("a\n", true)] + [TestCase("a\r\n", true)] + [TestCase("a\r", true)] + [TestCase("a\n\n", true)] + [TestCase("a", false)] + [TestCase("a ", false)] + [TestCase("", false)] + [TestCase(null, false)] + [Test] + public void HasTrailingNewLine_DetectsATrailingLineBreak(string input, bool expected) + { + input.HasTrailingNewLine().Should().Be(expected); + } + [TestCase("to_camel_case_function", "toCamelCaseFunction")] [TestCase("My S3 Bucket", "myS3Bucket")] [TestCase("-only-$AlphaNUMERIC-characters%^", "onlyAlphanumericCharacters")] diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 484e7c72ed..3e37462ecc 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Models; using Calamari.Common.Plumbing.Logging; @@ -56,9 +55,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(), new HashSet()); } - using var writer = new StringWriter(); - yamlStream.Save(writer, false); - var modifiedContent = writer.ToString().TrimEnd(); + var modifiedContent = YamlStreamLoader.SerializeDocuments(yamlStream.Documents, input); return new ImageReplacementResult(modifiedContent, allUpdatedImages, new HashSet()); } diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 8c1f2cd014..27948ca441 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Text; +using Calamari.Common.Plumbing.Extensions; using YamlDotNet.Core; using YamlDotNet.RepresentationModel; using YamlDotNet.Serialization; @@ -22,7 +23,7 @@ public HelmYamlParser(string yamlContent) var reader = new StringReader(yamlString); yamlStream = new YamlStream(); yamlStream.Load(reader); - endsWithNewline = yamlString.EndsWith(Environment.NewLine); + endsWithNewline = yamlString.HasTrailingNewLine(); } readonly string yamlString; @@ -87,6 +88,7 @@ string ReplaceNodeContent(YamlScalarNode node, string newValue) { var result = new StringBuilder(); using var reader = new StringReader(yamlString); + var newLine = yamlString.DetectLineEnding() ?? "\n"; var targetLine = (int)node.Start.Line; int startColumn; @@ -115,11 +117,11 @@ string ReplaceNodeContent(YamlScalarNode node, string newValue) // Replace in this line var before = line[..startColumn]; var after = line[endColumn..]; - result.AppendLine(before + newValue + after); + result.Append(before + newValue + after).Append(newLine); } else { - result.AppendLine(line); + result.Append(line).Append(newLine); } currentLine++; } diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index e7a0ec585c..72484df8c4 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -91,9 +91,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index 0796ea902c..426c28e44e 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -1,7 +1,6 @@ #nullable enable using System; using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; @@ -72,15 +71,9 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) - { - var singleDocStream = new YamlStream(stream.Documents[0]); - singleDocStream.Save(writer, false); - } - var modifiedContent = writer.ToString().TrimEnd(); + // Take just the first document to avoid unwanted document separators. + var modifiedContent = YamlStreamLoader.SerializeDocuments(stream.Documents.Take(1), yamlContent); return new ImageReplacementResult(modifiedContent, combinedResult.UpdatedImageReferences, combinedResult.AlreadyUpToDateImages); } diff --git a/source/Calamari/ArgoCD/YamlStreamLoader.cs b/source/Calamari/ArgoCD/YamlStreamLoader.cs index 83965b535a..e7b5cc00d9 100644 --- a/source/Calamari/ArgoCD/YamlStreamLoader.cs +++ b/source/Calamari/ArgoCD/YamlStreamLoader.cs @@ -109,27 +109,23 @@ public static string SerializeDocuments(IEnumerable documents, str return string.Empty; var newLine = originalContent?.DetectLineEnding() ?? "\n"; - var serializedDocs = new List(); + var serializedDocs = documentList.Select(doc => SerializeDocument(doc, newLine)); - foreach (var doc in documentList) - { - using var writer = new StringWriter(); - var tempStream = new YamlStream(doc); - tempStream.Save(writer, false); - var serialized = writer.ToString(); - - serialized = serialized.TrimEnd(); - if (serialized.EndsWith("...")) - { - serialized = serialized.Substring(0, serialized.Length - 3).TrimEnd(); - } - - serializedDocs.Add(serialized); - } + var joined = string.Join($"{newLine}---{newLine}", serializedDocs); + return originalContent.HasTrailingNewLine() ? joined + newLine : joined; + } - return documentList.Count == 1 - ? serializedDocs[0] - : string.Join($"{newLine}---{newLine}", serializedDocs); + static string SerializeDocument(YamlDocument document, string newLine) + { + // The emitter always writes '\n' regardless of the writer's NewLine, so the document's + // own line ending has to be reapplied afterwards. + using var writer = new StringWriter(); + new YamlStream(document).Save(writer, false); + + var serialized = writer.ToString().TrimEnd().ReplaceLineEndings(newLine); + return serialized.EndsWith("...") + ? serialized.Substring(0, serialized.Length - 3).TrimEnd() + : serialized; } } } \ No newline at end of file