Conversation
The MapLabelsToAreas method was splitting area names by comma, which broke
area names containing commas (e.g., "Alerting, connectors, and reporting").
Changes:
- Remove comma-splitting logic from PrInfoProcessor.MapLabelsToAreas
- Remove comma-splitting logic from GitHubReleaseChangelogService.MapLabelsToAreas
- Update ChangelogConfiguration.cs doc comment to clarify behavior
- Update changelog.example.yml comments with examples of mapping one label to multiple areas
- Add unit tests to verify area names with commas are preserved
- Add integration tests for the multi-area per label pattern
To map one label to multiple areas, repeat the label under each area:
areas:
"Search":
- "Team:Search"
"Observability":
- "Team:Search"
Made-with: Cursor
3 tasks
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
src/services/Elastic.Changelog/GithubRelease/GitHubReleaseChangelogService.cs
Fixed
Show fixed
Hide fixed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated label→area mapping to stop splitting mapped area names on commas: mapping functions in changelog services now preserve each configured area string as a single entry and iterate explicit lists instead of parsing comma-separated values. The changelog example comment was clarified to state area names may contain commas and that mapping a label to multiple areas requires repeating the label under each area key. Configuration loading and the 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Elastic.Documentation.Configuration/Changelog/ChangelogConfiguration.cs`:
- Around line 86-90: Change LabelToAreas from a single-string value to a
collection type (e.g. IReadOnlyDictionary<string, IReadOnlyCollection<string>>
or IReadOnlyDictionary<string, IEnumerable<string>>) so a label can map to
multiple areas; update the configuration loader that populates LabelToAreas and
both call sites of MapLabelsToAreas to expect the new collection-valued mapping
and to union/flatten all mapped areas for a label (rather than treating a single
comma-containing string as one area) so the
CreateChangelog_WithOneLabelMappedToMultipleAreas_AddsAllAreas test passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23f697cd-a7d3-4582-a5c2-52b99d8453ef
📒 Files selected for processing (5)
config/changelog.example.ymlsrc/Elastic.Documentation.Configuration/Changelog/ChangelogConfiguration.cssrc/services/Elastic.Changelog/Creation/PrInfoProcessor.cssrc/services/Elastic.Changelog/GithubRelease/GitHubReleaseChangelogService.cstests/Elastic.Changelog.Tests/Changelogs/Create/LabelMappingTests.cs
src/Elastic.Documentation.Configuration/Changelog/ChangelogConfiguration.cs
Outdated
Show resolved
Hide resolved
Comment on lines
+392
to
+399
| foreach (var label in labels) | ||
| { | ||
| if (!labelToAreasMapping.TryGetValue(label, out var mappedAreas)) | ||
| continue; | ||
|
|
||
| foreach (var area in mappedAreas) | ||
| _ = areas.Add(area); | ||
| } |
Comment on lines
+348
to
+355
| foreach (var label in labels) | ||
| { | ||
| if (!labelToAreasMapping.TryGetValue(label, out var mappedAreas)) | ||
| continue; | ||
|
|
||
| foreach (var area in mappedAreas) | ||
| _ = areas.Add(area); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2934
Summary
The
MapLabelsToAreasmethod was splitting area names by comma, which broke area names containing commas (e.g., "Alerting, connectors, and reporting").Changes
PrInfoProcessor.MapLabelsToAreasGitHubReleaseChangelogService.MapLabelsToAreasChangelogConfiguration.csdoc comment to clarify behaviorchangelog.example.ymlcomments with examples of mapping one label to multiple areasTo map one label to multiple areas, repeat the label under each area:
NOTE: Some changes were required on top of the original plan (in #2934) to address the following test failure:
Test fixes
The test expected one GitHub label (
Team:Search) listed under twopivot.areasentries to produce both areas.BuildLabelToAreasMappingusedDictionary<string, string>and didlabelToAreas[label] = areaName, so each label could only map to one area—the last area in file order won. That’s why the test failed even after removing the comma split.The fix is as follows:
BuildLabelToAreasMappingnow buildsDictionary<string, List<string>>and appends each area for a label (deduped case-insensitively).ChangelogConfiguration.LabelToAreasis nowIReadOnlyDictionary<string, IReadOnlyList<string>>?so each label can map to multiple areas.MapLabelsToAreas(inPrInfoProcessorandGitHubReleaseChangelogService) iterates every mapped area for each label.IReadOnlyDictionary<string, IReadOnlyList<string>>withlabelToAreas?.ToDictionary(...)(also fixes IDE0031).ChangelogConfigurationTestsassert with.ContainSingle().Whichwhere each label still maps to one area.LabelMappingTestsuseIReadOnlyList<string>in theMapLabelsToAreasunit test; the multi-area integration test keeps assertions- Search/- Observabilityto match emitted YAML (capitalized keys from your config).dotnet test tests/Elastic.Changelog.Testsnow reports 399 passed.Note: This is a public API change on
ChangelogConfiguration: anything that readLabelToAreas[label]as astringmust now treat the value asIReadOnlyList<string>(usually one element).Generative AI disclosure
Tool(s) and model(s) used: claude-4.5-haiku