diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs index 33b7bd3b4..1fa079929 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogEntry.cs @@ -27,6 +27,9 @@ public record ChangelogEntryDto [YamlMember(Alias = "feature-id", ApplyNamingConventions = false)] public string? FeatureId { get; set; } public bool? Highlight { get; set; } + + /// Bare PR number referencing the canonical entry. Marks this as a pipeline-written marker; must be the only field present. + public string? Link { get; set; } } /// diff --git a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs index a322ebb27..b780517ac 100644 --- a/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs +++ b/src/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cs @@ -165,7 +165,8 @@ private static string ToYamlDoubleQuotedString(string s) Impact = dto.Impact, Action = dto.Action, FeatureId = dto.FeatureId, - Highlight = dto.Highlight + Highlight = dto.Highlight, + Link = dto.Link }; private static ChangelogEntry ToEntry(BundledEntry entry) => new() @@ -181,7 +182,8 @@ private static string ToYamlDoubleQuotedString(string s) Impact = entry.Impact, Action = entry.Action, FeatureId = entry.FeatureId, - Highlight = entry.Highlight + Highlight = entry.Highlight, + Link = entry.Link }; private static ProductReference ToProductReference(ProductInfoDto dto) => new() @@ -294,7 +296,8 @@ private static ChangelogEntryType ParseEntryType(string? value) Impact = entry.Impact, Action = entry.Action, FeatureId = entry.FeatureId, - Highlight = entry.Highlight + Highlight = entry.Highlight, + Link = entry.Link }; private static ProductInfoDto ToDto(ProductReference product) => new() diff --git a/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs b/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs index e33c2cf83..6801ae8f3 100644 --- a/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/BundledEntry.cs @@ -47,4 +47,7 @@ public record BundledEntry /// Related issue URLs or references. public IReadOnlyList? Issues { get; init; } + + /// Bare PR number referencing the canonical entry. Present only on pipeline-written markers. + public string? Link { get; init; } } diff --git a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs index 28f8091ad..53fd67810 100644 --- a/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs +++ b/src/Elastic.Documentation/ReleaseNotes/ChangelogEntry.cs @@ -46,6 +46,9 @@ public record ChangelogEntry /// Whether this entry should be highlighted. public bool? Highlight { get; init; } + /// Bare PR number referencing the canonical entry. Marks this as a pipeline-written marker; must be the only field present. + public string? Link { get; init; } + /// /// Converts this ChangelogEntry to a BundledEntry for embedding in bundles. /// File property is set to null; set it separately using a 'with' expression. @@ -64,6 +67,7 @@ public record ChangelogEntry Subtype = Subtype, Areas = Areas, Prs = Prs, - Issues = Issues + Issues = Issues, + Link = Link }; } diff --git a/src/services/Elastic.Changelog/Bundling/ChangelogEntryMatcher.cs b/src/services/Elastic.Changelog/Bundling/ChangelogEntryMatcher.cs index 56f9f259a..4c8871b73 100644 --- a/src/services/Elastic.Changelog/Bundling/ChangelogEntryMatcher.cs +++ b/src/services/Elastic.Changelog/Bundling/ChangelogEntryMatcher.cs @@ -28,18 +28,37 @@ public async Task MatchChangelogsAsync( Cancel ctx) { var changelogEntries = new List(); + var markers = new List<(string FileName, string Link)>(); var matchedPrs = new HashSet(StringComparer.OrdinalIgnoreCase); var matchedIssues = new HashSet(StringComparer.OrdinalIgnoreCase); var seenChangelogs = new HashSet(); foreach (var filePath in yamlFiles) { - var entry = await ProcessFileAsync(collector, filePath, criteria, seenChangelogs, matchedPrs, matchedIssues, ctx); + ctx.ThrowIfCancellationRequested(); + string content; + try + { + content = await fileSystem.File.ReadAllTextAsync(filePath, ctx); + } + catch (Exception ex) when (ex is not (OperationCanceledException or OutOfMemoryException or StackOverflowException or ThreadAbortException)) + { + logger.LogWarning(ex, "Error reading file {FilePath}", filePath); + collector.EmitError(filePath, $"Error processing file: {ex.Message}"); + continue; + } + var fileName = fileSystem.Path.GetFileName(filePath); + if (TryExtractMarkerLink(content, out var link)) + { + markers.Add((fileName, link!)); + continue; + } + var entry = ProcessContent(collector, filePath, fileName, content, criteria, seenChangelogs, matchedPrs, matchedIssues); if (entry != null) changelogEntries.Add(entry); } - return BuildResult(collector, changelogEntries, criteria, matchedPrs, matchedIssues); + return BuildResult(collector, changelogEntries, markers, criteria, matchedPrs, matchedIssues); } /// @@ -54,6 +73,7 @@ public ChangelogMatchResult MatchChangelogContents( Cancel ctx) { var changelogEntries = new List(); + var markers = new List<(string FileName, string Link)>(); var matchedPrs = new HashSet(StringComparer.OrdinalIgnoreCase); var matchedIssues = new HashSet(StringComparer.OrdinalIgnoreCase); var seenChangelogs = new HashSet(); @@ -61,21 +81,29 @@ public ChangelogMatchResult MatchChangelogContents( foreach (var (fileName, content) in contents) { ctx.ThrowIfCancellationRequested(); + if (TryExtractMarkerLink(content, out var link)) + { + markers.Add((fileName, link!)); + continue; + } var entry = ProcessContent(collector, fileName, fileName, content, criteria, seenChangelogs, matchedPrs, matchedIssues); if (entry != null) changelogEntries.Add(entry); } - return BuildResult(collector, changelogEntries, criteria, matchedPrs, matchedIssues); + return BuildResult(collector, changelogEntries, markers, criteria, matchedPrs, matchedIssues); } private static ChangelogMatchResult BuildResult( IDiagnosticsCollector collector, List changelogEntries, + List<(string FileName, string Link)> markers, ChangelogFilterCriteria criteria, HashSet matchedPrs, HashSet matchedIssues) { + ResolveMarkers(collector, markers, changelogEntries); + if (criteria.PrsToMatch.Count > 0) { foreach (var pr in criteria.PrsToMatch.Where(pr => !matchedPrs.Contains(pr))) @@ -96,29 +124,48 @@ private static ChangelogMatchResult BuildResult( }; } - private async Task ProcessFileAsync( + private static void ResolveMarkers( IDiagnosticsCollector collector, - string filePath, - ChangelogFilterCriteria criteria, - HashSet seenChangelogs, - HashSet matchedPrs, - HashSet matchedIssues, - Cancel ctx) + List<(string FileName, string Link)> markers, + List entries) { - string fileContent; - try + foreach (var (markerFile, link) in markers) { - fileContent = await fileSystem.File.ReadAllTextAsync(filePath, ctx); - } - catch (Exception ex) when (ex is not (OperationCanceledException or OutOfMemoryException or StackOverflowException or ThreadAbortException)) - { - logger.LogWarning(ex, "Error reading file {FilePath}", filePath); - collector.EmitError(filePath, $"Error processing file: {ex.Message}"); - return null; + var parent = entries.FirstOrDefault(e => + e.FileName.Equals($"{link}.yaml", StringComparison.OrdinalIgnoreCase) || + e.FileName.Equals($"{link}.yml", StringComparison.OrdinalIgnoreCase)); + + if (parent == null) + { + collector.EmitError(string.Empty, + $"Marker '{markerFile}' references PR {link} but no entry for that PR was found. " + + "The canonical entry may not have been uploaded yet."); + continue; + } + + if (parent.Data.Link != null) + { + collector.EmitError(string.Empty, + $"Marker '{markerFile}' references '{parent.FileName}' which is itself a marker. " + + "Marker chains (depth > 1) are not supported."); + } + // Parent is already included in entries; the marker contributes no new entry. } + } - var fileName = fileSystem.Path.GetFileName(filePath); - return ProcessContent(collector, filePath, fileName, fileContent, criteria, seenChangelogs, matchedPrs, matchedIssues); + private static bool TryExtractMarkerLink(string content, out string? link) + { + link = null; + var normalized = content.Trim(); + if (!normalized.StartsWith("link:", StringComparison.OrdinalIgnoreCase)) + return false; + + var value = normalized["link:".Length..].Trim(); + if (value.Contains('\n') || string.IsNullOrWhiteSpace(value)) + return false; + + link = value; + return true; } private MatchedChangelogFile? ProcessContent( diff --git a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs index 9fd627e51..883872422 100644 --- a/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs +++ b/src/services/Elastic.Changelog/Scrubbing/ChangelogContentScrubber.cs @@ -74,6 +74,19 @@ private async Task ScrubChangelog(string content, Cancel ctx) var normalized = ReleaseNotesSerialization.NormalizeYaml(content); var entry = ReleaseNotesSerialization.DeserializeEntry(normalized); + // Marker: link: with no other content. Return unchanged — there is no URL to scrub. + if (entry.Link != null) + { + var hasContent = !string.IsNullOrEmpty(entry.Title) + || entry.Type != ChangelogEntryType.Invalid + || entry.Products is { Count: > 0 } + || entry.Prs is { Count: > 0 }; + if (hasContent) + throw new InvalidOperationException( + "Changelog entry has both 'link:' and content fields. A marker must contain only 'link: '."); + return content; + } + var bundledEntry = new BundledEntry { Type = entry.Type, diff --git a/tests/Elastic.Changelog.Tests/Changelogs/MarkerResolutionTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/MarkerResolutionTests.cs new file mode 100644 index 000000000..8ed08f758 --- /dev/null +++ b/tests/Elastic.Changelog.Tests/Changelogs/MarkerResolutionTests.cs @@ -0,0 +1,132 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information + +using System.IO.Abstractions.TestingHelpers; +using AwesomeAssertions; +using Elastic.Changelog.Bundling; +using Elastic.Documentation; +using Elastic.Documentation.Configuration.ReleaseNotes; +using Elastic.Documentation.Diagnostics; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Elastic.Changelog.Tests.Changelogs; + +/// +/// Verifies that handles link: markers correctly: +/// markers are excluded from bundle output; the parent entry is included once; +/// missing parents and chained markers are hard errors. +/// +public class MarkerResolutionTests +{ + // language=yaml + private const string RealEntry = + """ + title: Fix the thing + type: bug-fix + prs: + - "https://github.com/elastic/elasticsearch/pull/100" + products: + - product: elasticsearch + target: 9.3.0 + lifecycle: ga + """; + + private static ChangelogEntryMatcher BuildMatcher() => + new(new MockFileSystem(), ReleaseNotesSerialization.GetEntryDeserializer(), NullLogger.Instance); + + private static ChangelogFilterCriteria AllEntries() => + new() + { + IncludeAll = true, + ProductFilters = [], + PrsToMatch = [], + IssuesToMatch = [] + }; + + private Cancel Ctx => TestContext.Current.CancellationToken; + + [Fact] + public async Task Marker_IsExcludedFromOutput_ParentIncludedOnce() + { + var matcher = BuildMatcher(); + var contents = new List<(string FileName, string Content)> + { + ("100.yaml", RealEntry), + ("200.yaml", "link: 100\n") + }; + await using var collector = new DiagnosticsCollector([]); + + var result = matcher.MatchChangelogContents(collector, contents, AllEntries(), Ctx); + + result.Entries.Should().HaveCount(1, "marker must be suppressed from bundle output"); + result.Entries[0].FileName.Should().Be("100.yaml"); + collector.Errors.Should().Be(0, "no errors when parent is found"); + } + + [Fact] + public async Task Marker_MissingParent_EmitsError() + { + var matcher = BuildMatcher(); + var contents = new List<(string FileName, string Content)> + { + ("200.yaml", "link: 100\n") + }; + await using var collector = new DiagnosticsCollector([]); + + _ = matcher.MatchChangelogContents(collector, contents, AllEntries(), Ctx); + + collector.Errors.Should().BeGreaterThan(0, "a marker with no parent is a hard error"); + } + + [Fact] + public async Task Marker_PointingAtAnotherMarker_EmitsError() + { + var matcher = BuildMatcher(); + var contents = new List<(string FileName, string Content)> + { + // 100.yaml is itself a marker → parent is a marker → depth > 1 + ("100.yaml", "link: 50\n"), + ("200.yaml", "link: 100\n") + }; + await using var collector = new DiagnosticsCollector([]); + + _ = matcher.MatchChangelogContents(collector, contents, AllEntries(), Ctx); + + collector.Errors.Should().BeGreaterThan(0, "marker chains (depth > 1) must error"); + } + + [Fact] + public async Task TwoMarkers_SameParent_OneEntryInOutput() + { + var matcher = BuildMatcher(); + var contents = new List<(string FileName, string Content)> + { + ("100.yaml", RealEntry), + ("200.yaml", "link: 100\n"), + ("300.yaml", "link: 100\n") + }; + await using var collector = new DiagnosticsCollector([]); + + var result = matcher.MatchChangelogContents(collector, contents, AllEntries(), Ctx); + + result.Entries.Should().HaveCount(1, "both markers are suppressed; parent appears once"); + collector.Errors.Should().Be(0); + } + + [Fact] + public async Task NoMarkers_NormalEntries_Unaffected() + { + var matcher = BuildMatcher(); + var contents = new List<(string FileName, string Content)> + { + ("100.yaml", RealEntry) + }; + await using var collector = new DiagnosticsCollector([]); + + var result = matcher.MatchChangelogContents(collector, contents, AllEntries(), Ctx); + + result.Entries.Should().HaveCount(1); + collector.Errors.Should().Be(0); + } +} diff --git a/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs new file mode 100644 index 000000000..47c20149b --- /dev/null +++ b/tests/Elastic.Changelog.Tests/Changelogs/MarkerScrubTests.cs @@ -0,0 +1,87 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information + +using AwesomeAssertions; +using Elastic.Changelog.Scrubbing; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Elastic.Changelog.Tests.Changelogs; + +/// +/// Verifies that handles link: markers correctly: +/// a bare marker passes through unchanged; a marker with content fields throws. +/// +public class MarkerScrubTests +{ + private readonly IChangelogContentScrubber _scrubber = + new ChangelogContentScrubber(NullLoggerFactory.Instance, ["elastic/elasticsearch"]); + + private Cancel Ctx => TestContext.Current.CancellationToken; + + [Fact] + public async Task Marker_OnlyLink_PassesThroughUnchanged() + { + const string key = "changelog/elastic/elasticsearch/main/200.yaml"; + const string content = "link: 100\n"; + + var result = await _scrubber.ScrubAsync(key, content, Ctx); + + result.Should().Be(content); + } + + [Fact] + public async Task Marker_OnlyLink_NoTrailingNewline_PassesThroughUnchanged() + { + const string key = "changelog/elastic/elasticsearch/main/200.yaml"; + const string content = "link: 100"; + + var result = await _scrubber.ScrubAsync(key, content, Ctx); + + result.Should().Be(content); + } + + [Fact] + public async Task Marker_WithTitle_ThrowsInvalidOperation() + { + const string key = "changelog/elastic/elasticsearch/main/200.yaml"; + const string content = """ + link: 100 + title: This is wrong + """; + + var act = async () => await _scrubber.ScrubAsync(key, content, Ctx); + + await act.Should().ThrowAsync() + .WithMessage("*link:*content fields*"); + } + + [Fact] + public async Task Marker_WithType_ThrowsInvalidOperation() + { + const string key = "changelog/elastic/elasticsearch/main/200.yaml"; + const string content = """ + link: 100 + type: bug-fix + """; + + var act = async () => await _scrubber.ScrubAsync(key, content, Ctx); + + await act.Should().ThrowAsync(); + } + + [Fact] + public async Task Marker_WithPrs_ThrowsInvalidOperation() + { + const string key = "changelog/elastic/elasticsearch/main/200.yaml"; + const string content = """ + link: 100 + prs: + - "https://github.com/elastic/elasticsearch/pull/100" + """; + + var act = async () => await _scrubber.ScrubAsync(key, content, Ctx); + + await act.Should().ThrowAsync(); + } +}