Skip to content

Commit 813df79

Browse files
authored
Fix area name comma-split bug in changelog label mapping (#2935)
1 parent 771dd4d commit 813df79

7 files changed

Lines changed: 207 additions & 31 deletions

File tree

config/changelog.example.yml

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,22 @@ pivot:
104104
# - ">release-highlight"
105105

106106
# Area definitions with labels
107-
# Keys are area display names, values are label strings or lists
107+
# Keys are area display names (can contain commas), values are label strings or lists
108+
# Each label maps to exactly one area.
108109
# String form: "label1, label2" | List form: [label1, label2]
110+
# To map one label to multiple areas, repeat the label under each area name.
109111
areas:
110112
# Example mappings - customize based on your label naming conventions
111113
# Autoscaling: ":Distributed Coordination/Autoscaling"
112114
# Search: ":Search/Search"
113115
# Security: ":Security/Security"
114116
# Watcher: ":Data Management/Watcher"
117+
# To map a label to multiple areas (e.g., "Team:Search" to both "Search" and "Observability"):
118+
# Search:
119+
# - ":Search/Search"
120+
# - "Team:Search"
121+
# Observability:
122+
# - "Team:Search"
115123
# List form example:
116124
# Search:
117125
# - ":Search/Search"

src/Elastic.Documentation.Configuration/Changelog/ChangelogConfiguration.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,9 @@ public record ChangelogConfiguration
8585

8686
/// <summary>
8787
/// Mapping from GitHub label names to changelog area values (computed from Pivot.Areas)
88-
/// Multiple labels can map to the same area, and a single label can map to multiple areas (comma-separated)
88+
/// Multiple labels can map to the same area. To map one label to multiple areas, repeat the label under each area in pivot.areas.
8989
/// </summary>
90-
public IReadOnlyDictionary<string, string>? LabelToAreas { get; init; }
90+
public IReadOnlyDictionary<string, IReadOnlyList<string>>? LabelToAreas { get; init; }
9191

9292
/// <summary>
9393
/// Mapping from GitHub label names to product spec strings (computed from Pivot.Products).

src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ internal static ChangelogConfigurationYaml DeserializeConfiguration(string yaml)
115115
IReadOnlyList<string> availableSubtypes;
116116
IReadOnlyList<string>? availableAreas;
117117
Dictionary<string, string>? labelToType;
118-
Dictionary<string, string>? labelToAreas;
118+
Dictionary<string, List<string>>? labelToAreas;
119119
Dictionary<string, string>? labelToProducts;
120120
PivotConfiguration? pivot = null;
121121

@@ -309,6 +309,11 @@ internal static ChangelogConfigurationYaml DeserializeConfiguration(string yaml)
309309
filenameStrategy = parsed;
310310
}
311311

312+
var labelToAreasReadOnly = labelToAreas?.ToDictionary(
313+
kvp => kvp.Key,
314+
kvp => (IReadOnlyList<string>)kvp.Value,
315+
StringComparer.OrdinalIgnoreCase);
316+
312317
return new ChangelogConfiguration
313318
{
314319
Pivot = pivot,
@@ -318,7 +323,7 @@ internal static ChangelogConfigurationYaml DeserializeConfiguration(string yaml)
318323
Areas = availableAreas,
319324
Products = products,
320325
LabelToType = labelToType,
321-
LabelToAreas = labelToAreas,
326+
LabelToAreas = labelToAreasReadOnly,
322327
LabelToProducts = labelToProducts,
323328
Rules = rules,
324329
HighlightLabels = highlightLabels,
@@ -1010,22 +1015,31 @@ private static BundleConfiguration ParseBundleConfiguration(BundleConfigurationY
10101015

10111016
/// <summary>
10121017
/// Builds LabelToAreas mapping by inverting pivot.areas entries.
1013-
/// Each label in an area entry maps to that area name.
1018+
/// Each label in an area entry maps to that area name. The same label may appear under multiple areas; all area names are collected.
10141019
/// </summary>
1015-
private static Dictionary<string, string>? BuildLabelToAreasMapping(Dictionary<string, YamlLenientList?>? areas)
1020+
private static Dictionary<string, List<string>>? BuildLabelToAreasMapping(Dictionary<string, YamlLenientList?>? areas)
10161021
{
10171022
if (areas == null || areas.Count == 0)
10181023
return null;
10191024

1020-
var labelToAreas = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
1025+
var labelToAreas = new Dictionary<string, List<string>>(StringComparer.OrdinalIgnoreCase);
10211026

10221027
foreach (var (areaName, labelList) in areas)
10231028
{
10241029
if (labelList?.Values == null)
10251030
continue;
10261031

10271032
foreach (var label in labelList.Values)
1028-
labelToAreas[label] = areaName;
1033+
{
1034+
if (!labelToAreas.TryGetValue(label, out var areaNames))
1035+
{
1036+
areaNames = [];
1037+
labelToAreas[label] = areaNames;
1038+
}
1039+
1040+
if (!areaNames.Exists(a => a.Equals(areaName, StringComparison.OrdinalIgnoreCase)))
1041+
areaNames.Add(areaName);
1042+
}
10291043
}
10301044

10311045
return labelToAreas.Count > 0 ? labelToAreas : null;

src/services/Elastic.Changelog/Creation/PrInfoProcessor.cs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -386,15 +386,17 @@ internal static bool IsBlockedByRules(string[] prLabels, CreateRules rules)
386386
.Select(label => labelToTypeMapping.TryGetValue(label, out var mappedType) ? mappedType : null)
387387
.FirstOrDefault(mappedType => mappedType != null);
388388

389-
internal static List<string> MapLabelsToAreas(string[] labels, IReadOnlyDictionary<string, string> labelToAreasMapping)
389+
internal static List<string> MapLabelsToAreas(string[] labels, IReadOnlyDictionary<string, IReadOnlyList<string>> labelToAreasMapping)
390390
{
391391
var areas = new HashSet<string>();
392-
var areaList = labels
393-
.Where(label => labelToAreasMapping.ContainsKey(label))
394-
.SelectMany(label => labelToAreasMapping[label]
395-
.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries));
396-
foreach (var area in areaList)
397-
_ = areas.Add(area);
392+
foreach (var label in labels)
393+
{
394+
if (!labelToAreasMapping.TryGetValue(label, out var mappedAreas))
395+
continue;
396+
397+
foreach (var area in mappedAreas)
398+
_ = areas.Add(area);
399+
}
398400
return areas.ToList();
399401
}
400402

src/services/Elastic.Changelog/GithubRelease/GitHubReleaseChangelogService.cs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -346,15 +346,17 @@ private static string GenerateYaml(ChangelogEntry data) =>
346346
.Select(label => labelToTypeMapping.TryGetValue(label, out var mappedType) ? mappedType : null)
347347
.FirstOrDefault(mappedType => mappedType != null);
348348

349-
private static List<string> MapLabelsToAreas(string[] labels, IReadOnlyDictionary<string, string> labelToAreasMapping)
349+
private static List<string> MapLabelsToAreas(string[] labels, IReadOnlyDictionary<string, IReadOnlyList<string>> labelToAreasMapping)
350350
{
351351
var areas = new HashSet<string>();
352-
var areaList = labels
353-
.Where(label => labelToAreasMapping.ContainsKey(label))
354-
.SelectMany(label => labelToAreasMapping[label]
355-
.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries));
356-
foreach (var area in areaList)
357-
_ = areas.Add(area);
352+
foreach (var label in labels)
353+
{
354+
if (!labelToAreasMapping.TryGetValue(label, out var mappedAreas))
355+
continue;
356+
357+
foreach (var area in mappedAreas)
358+
_ = areas.Add(area);
359+
}
358360
return areas.ToList();
359361
}
360362

tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,9 @@ public async Task LoadChangelogConfiguration_WithPivotAreas_ComputesLabelToAreas
191191
// Should have inverted label mappings
192192
config.LabelToAreas.Should().NotBeNull();
193193
config.LabelToAreas.Should().ContainKey(":Search/Search");
194-
config.LabelToAreas[":Search/Search"].Should().Be("Search");
194+
config.LabelToAreas[":Search/Search"].Should().ContainSingle().Which.Should().Be("Search");
195195
config.LabelToAreas.Should().ContainKey(":Security/Security");
196-
config.LabelToAreas[":Security/Security"].Should().Be("Security");
196+
config.LabelToAreas[":Security/Security"].Should().ContainSingle().Which.Should().Be("Security");
197197
}
198198
finally
199199
{
@@ -695,12 +695,12 @@ public async Task LoadChangelogConfiguration_PivotAreas_AsListValues_ComputesMap
695695
// Both labels from the list should map to "Search"
696696
config.LabelToAreas.Should().NotBeNull();
697697
config.LabelToAreas.Should().ContainKey(":Search/Search");
698-
config.LabelToAreas[":Search/Search"].Should().Be("Search");
698+
config.LabelToAreas[":Search/Search"].Should().ContainSingle().Which.Should().Be("Search");
699699
config.LabelToAreas.Should().ContainKey(":Search/Ranking");
700-
config.LabelToAreas[":Search/Ranking"].Should().Be("Search");
700+
config.LabelToAreas[":Search/Ranking"].Should().ContainSingle().Which.Should().Be("Search");
701701
// String form should still work
702702
config.LabelToAreas.Should().ContainKey(":Security/Security");
703-
config.LabelToAreas[":Security/Security"].Should().Be("Security");
703+
config.LabelToAreas[":Security/Security"].Should().ContainSingle().Which.Should().Be("Security");
704704
}
705705

706706
[Fact]
@@ -860,11 +860,11 @@ public async Task LoadChangelogConfiguration_MixedStringAndListForms_ParsesCorre
860860

861861
// Areas: string for Search, list for Security
862862
config.LabelToAreas.Should().ContainKey(":Search/Search");
863-
config.LabelToAreas[":Search/Search"].Should().Be("Search");
863+
config.LabelToAreas[":Search/Search"].Should().ContainSingle().Which.Should().Be("Search");
864864
config.LabelToAreas.Should().ContainKey(":Security/Security");
865-
config.LabelToAreas[":Security/Security"].Should().Be("Security");
865+
config.LabelToAreas[":Security/Security"].Should().ContainSingle().Which.Should().Be("Security");
866866
config.LabelToAreas.Should().ContainKey(":Security/Auth");
867-
config.LabelToAreas[":Security/Auth"].Should().Be("Security");
867+
config.LabelToAreas[":Security/Auth"].Should().ContainSingle().Which.Should().Be("Security");
868868
}
869869

870870
/// <summary>

tests/Elastic.Changelog.Tests/Changelogs/Create/LabelMappingTests.cs

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,4 +432,154 @@ public async Task CreateChangelog_WithPrOptionAndAreaMapping_MapsLabelsToAreas()
432432
yamlContent.Should().Contain("- security");
433433
yamlContent.Should().Contain("- search");
434434
}
435+
436+
[Fact]
437+
public void MapLabelsToAreas_WithAreaNameContainingCommas_PresservesFullName()
438+
{
439+
// Arrange
440+
var labelToAreas = new Dictionary<string, IReadOnlyList<string>>(StringComparer.OrdinalIgnoreCase)
441+
{
442+
["Team:Alerting Services"] = ["Alerting, connectors, and reporting"],
443+
["Team:Search"] = ["Search"]
444+
};
445+
446+
// Act
447+
var result = PrInfoProcessor.MapLabelsToAreas(
448+
["Team:Alerting Services", "Team:Search"],
449+
labelToAreas
450+
);
451+
452+
// Assert
453+
result.Should().HaveCount(2);
454+
result.Should().Contain("Alerting, connectors, and reporting"); // Not split
455+
result.Should().Contain("Search");
456+
}
457+
458+
[Fact]
459+
public async Task CreateChangelog_WithAreaNameContainingCommas_PreservesAreaName()
460+
{
461+
// Arrange: Area name contains commas
462+
var prInfo = new GitHubPrInfo
463+
{
464+
Title = "Fix alerting issues",
465+
Labels = ["type:bug", "Team:Alerting Services"]
466+
};
467+
468+
A.CallTo(() => MockGitHubService.FetchPrInfoAsync(
469+
A<string>._,
470+
A<string?>._,
471+
A<string?>._,
472+
A<CancellationToken>._))
473+
.Returns(prInfo);
474+
475+
// language=yaml
476+
var configContent =
477+
"""
478+
pivot:
479+
types:
480+
bug-fix: "type:bug"
481+
feature:
482+
breaking-change:
483+
areas:
484+
"Alerting, connectors, and reporting":
485+
- "Team:Alerting Services"
486+
lifecycles:
487+
- preview
488+
- beta
489+
- ga
490+
""";
491+
var configPath = await CreateConfigDirectory(configContent);
492+
493+
var service = CreateService();
494+
495+
var input = new CreateChangelogArguments
496+
{
497+
Prs = ["https://github.com/elastic/elasticsearch/pull/12345"],
498+
Products = [new ProductArgument { Product = "elasticsearch", Target = "9.2.0" }],
499+
Config = configPath,
500+
Output = CreateOutputDirectory()
501+
};
502+
503+
// Act
504+
var result = await service.CreateChangelog(Collector, input, TestContext.Current.CancellationToken);
505+
506+
// Assert
507+
if (!result)
508+
{
509+
foreach (var diagnostic in Collector.Diagnostics)
510+
Output.WriteLine($"{diagnostic.Severity}: {diagnostic.Message}");
511+
}
512+
513+
result.Should().BeTrue();
514+
Collector.Errors.Should().Be(0);
515+
516+
var outputDir = input.Output ?? FileSystem.Directory.GetCurrentDirectory();
517+
if (!FileSystem.Directory.Exists(outputDir))
518+
FileSystem.Directory.CreateDirectory(outputDir);
519+
var files = FileSystem.Directory.GetFiles(outputDir, "*.yaml");
520+
var yamlContent = await FileSystem.File.ReadAllTextAsync(files[0], TestContext.Current.CancellationToken);
521+
yamlContent.Should().Contain("- Alerting, connectors, and reporting"); // Full name, not split
522+
}
523+
524+
[Fact]
525+
public async Task CreateChangelog_WithOneLabelMappedToMultipleAreas_AddsAllAreas()
526+
{
527+
// Arrange: Same label under multiple areas
528+
var prInfo = new GitHubPrInfo
529+
{
530+
Title = "Cross-area change",
531+
Labels = ["type:enhancement", "Team:Search"]
532+
};
533+
534+
A.CallTo(() => MockGitHubService.FetchPrInfoAsync(
535+
A<string>._,
536+
A<string?>._,
537+
A<string?>._,
538+
A<CancellationToken>._))
539+
.Returns(prInfo);
540+
541+
// language=yaml
542+
var configContent =
543+
"""
544+
pivot:
545+
types:
546+
enhancement: "type:enhancement"
547+
feature:
548+
bug-fix:
549+
breaking-change:
550+
areas:
551+
"Search":
552+
- "Team:Search"
553+
"Observability":
554+
- "Team:Search"
555+
lifecycles:
556+
- ga
557+
""";
558+
var configPath = await CreateConfigDirectory(configContent);
559+
560+
var service = CreateService();
561+
562+
var input = new CreateChangelogArguments
563+
{
564+
Prs = ["https://github.com/elastic/elasticsearch/pull/12345"],
565+
Products = [new ProductArgument { Product = "elasticsearch" }],
566+
Config = configPath,
567+
Output = CreateOutputDirectory()
568+
};
569+
570+
// Act
571+
var result = await service.CreateChangelog(Collector, input, TestContext.Current.CancellationToken);
572+
573+
// Assert
574+
result.Should().BeTrue();
575+
Collector.Errors.Should().Be(0);
576+
577+
var outputDir = input.Output;
578+
if (!FileSystem.Directory.Exists(outputDir))
579+
FileSystem.Directory.CreateDirectory(outputDir);
580+
var files = FileSystem.Directory.GetFiles(outputDir, "*.yaml");
581+
var yamlContent = await FileSystem.File.ReadAllTextAsync(files[0], TestContext.Current.CancellationToken);
582+
yamlContent.Should().Contain("- Search");
583+
yamlContent.Should().Contain("- Observability");
584+
}
435585
}

0 commit comments

Comments
 (0)