Improve changelog rule logic when config must apply to multiple products#2961
Open
Improve changelog rule logic when config must apply to multiple products#2961
Conversation
🔍 Preview links for changed docs |
lcawl
force-pushed
the
rules-bundle-products
branch
from
d2d8f12 to
804d0c0
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR separates changelog collection from bundle-time filtering so Sequence Diagram(s)mermaid Collector->>Entry: Gather changelog files (--input-products / --prs / --all) Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/changelog.example.yml`:
- Line 216: Update the warning comment about exclude_products to state that
per-context exclude_products override global product filtering, so context
excludes should generally be supersets of global excludes (include global
excludes) to avoid re-including globally excluded products; locate the comment
referencing "Context product lists should be subsets of global lists to avoid
conflicts" and change its wording to reflect that exclude_products in a context
replaces global excludes and therefore context lists should usually include
global excludes (be supersets) to prevent unintended inclusion.
In `@docs/contribute/changelog.md`:
- Around line 177-186: The docs for rules.bundle.products currently claim
per-product rules completely override global bundle rules; update the wording to
explain that overrides are applied per-dimension so unspecified dimensions fall
back to global rules (e.g., a product rule that only sets
include_products/exclude_products will not suppress global
exclude_types/include_types/exclude_areas/include_areas/match_areas); reference
the rules.bundle.products section and mirror this per-dimension fallback
clarification in the other affected section (the similar text around lines
221-239) so behavior aligns with tests (see test expectations in
BundleChangelogsTests regarding global exclude_types still applying).
In `@src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs`:
- Around line 759-768: The current fallback when candidates.Count == 0 uses
entrySet (which can pick another per-product override); instead, replace that
branch so it falls back to the global bundle rule: when candidates is empty set
it to a single-item list that contains the canonical global product key (e.g.
the existing GlobalProductKey constant or string.Empty/NULL key your byProduct
map uses for global rules) rather than entrySet, so the subsequent
byProduct.TryGetValue lookup returns the global rule; update the branch that
touches candidates, entrySet and byProduct in ChangelogBundlingService/Select...
logic accordingly.
In
`@src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs`:
- Around line 738-748: The per-context default for match_products is wrong:
change the initialization of contextMatchProducts from inheritedMatch to the
resolved bundle-level matchProducts so contexts inherit
rules.bundle.match_products by default; then if productYaml.MatchProducts is
non-empty, call ParseMatchMode(productYaml.MatchProducts), emit the same error
via collector.EmitError on failure, and set contextMatchProducts = parsed.Value
on success (keeping the existing ParseMatchMode and collector.EmitError usage);
ensure you update the assignment that currently uses inheritedMatch to use
matchProducts instead.
In `@tests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cs`:
- Around line 4839-4889: The test
BundleChangelogs_WithPerProductExcludeProducts_ExcludesContextMatchingProducts
currently doesn't verify that rules.bundle.products.security.exclude_products
actually exclude entries; add a new changelog entry created via CreateTestEntry
(e.g., "security-kibana-feature.yaml") whose product list includes both
"security" and "kibana" (shared [security, kibana] context), rerun the
BundleChangelogs call with the same input, and add an assertion that the output
bundle (read from outputPath) does NOT contain that new file name to ensure the
security-context exclude_products rule removed it; keep other assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73633035-ddcb-459a-94b0-bb5d91c07606
📒 Files selected for processing (9)
config/changelog.example.ymldocs/cli/release/changelog-bundle.mddocs/cli/release/changelog-gh-release.mddocs/contribute/changelog.mdsrc/Elastic.Documentation.Configuration/Changelog/BlockConfiguration.cssrc/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cssrc/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cstests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs
src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
Outdated
Show resolved
Hide resolved
src/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cs
Outdated
Show resolved
Hide resolved
coderabbitai
bot
added
documentation
coderabbitai
bot
added
documentation
|
Label error. Requires exactly 1 of: automation, breaking, bug, changelog:skip, chore, ci, dependencies, documentation, enhancement, feature, fix, redesign. Found: documentation, fix |
1 similar comment
|
Label error. Requires exactly 1 of: automation, breaking, bug, changelog:skip, chore, ci, dependencies, documentation, enhancement, feature, fix, redesign. Found: documentation, fix |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs (1)
714-785: Minor:ruleStatsmixes counters with a warning flag, producing odd summary output.Line 750 sets
ruleStats["ineffective_pattern_warned"] = 1, which will appear in the bundle summary at line 779 as "ineffective_pattern_warned: 1 entries". This is technically correct but confusing.Consider using a separate boolean flag for the warning state:
+var ineffectivePatternWarned = false; // ... -if (!ruleStats.ContainsKey("ineffective_pattern_warned")) +if (!ineffectivePatternWarned) { // emit hint - ruleStats["ineffective_pattern_warned"] = 1; + ineffectivePatternWarned = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs` around lines 714 - 785, The ruleStats dictionary is being used both for numeric counters and a boolean flag ("ineffective_pattern_warned") which yields confusing summary output; change the implementation in the ChangelogBundlingService loop to keep ruleStats strictly numeric and introduce a separate boolean (e.g., ineffectivePatternWarned) used by the per-product branch (where resolveResult.Rule.MatchProducts is checked) to ensure the collector.EmitHint for the ineffective-pattern warning is emitted only once; update the final summary assembly (where ruleStats is read and the message variable is built) to not include the boolean, and ensure references to ruleStats and the new ineffectivePatternWarned flag are used in ResolvePerProductBundleRule/ShouldExcludeByResolvedProductRule handling blocks as needed.tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs (1)
1171-1193: AppendAsyncto the new async test names.These added
[Fact]methods are allpublic async Taskmethods but still omit the requiredAsyncsuffix.As per coding guidelines, "Public async methods must use
PascalCase+Asyncsuffix (e.g.,GetDataAsync)".Also applies to: 1440-1610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs` around lines 1171 - 1193, The test method LoadChangelogConfiguration_WithRulesBundle_MatchProductsConjunction_LoadsCorrectly is a public async Task and must be renamed to include the Async suffix (e.g., LoadChangelogConfiguration_WithRulesBundle_MatchProductsConjunction_LoadsCorrectlyAsync); update the method name and any references to it (test discovery uses the method name) and apply the same renaming pattern to the other public async tests in this file (the ones noted in the comment range) so all public async methods follow PascalCase + Async convention.
🤖 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/services/Elastic.Changelog/Bundling/ProfileFilterResolver.cs`:
- Around line 341-350: The parser currently silently ignores extra tokens in
entries; update the logic that builds ProductArgument (the code handling
variable parts and the products.Add call) to validate token count: after
splitting into parts, if parts.Length is 0 continue, if parts.Length > 3 throw a
descriptive FormatException/ArgumentException that includes the offending entry
string, and only then create ProductArgument using parts[0..2] with default "*"
for missing tokens; this enforces fail-fast behavior for malformed product
entries in the ProductArgument creation path.
In `@tests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cs`:
- Around line 4366-4369: The assertion is checking for the wrong diagnostic tag;
update the diagnostic check in the test (the block that asserts
Collector.Diagnostics contains a message) to look for "[-bundle-global]" instead
of "[-bundle-exclude]" so it matches the bundle service's global product-filter
diagnostic; keep the rest of the predicate (e.g., checking for
"1755268130-elasticsearch-feature.yaml") and leave the surrounding assertions on
result and Collector.Errors unchanged.
- Around line 5053-5104: The test
BundleChangelogs_WithPerProductRules_FallsBackToGlobalWhenNoContextRule
currently passes under both old and new fallback behaviors because globals
include "security"; update the test so globals do NOT include "security" (e.g.,
change the top-level rules.bundle.include_products to only "elasticsearch") and
keep OutputProducts = [new ProductArgument { Product = "security", Target =
"9.3.0" }]; this way security-feature.yaml will only be included if
missing-context per-product rules are correctly bypassing global filters; adjust
the assertions to expect security-feature.yaml included and confirm that
globals-excluded entries remain excluded using
ServiceWithConfig.BundleChangelogs and BundleChangelogsArguments/ProductArgument
references.
In `@tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs`:
- Around line 1518-1549: The test
LoadChangelogConfiguration_WithPerProductProductFiltering_SubsetValidationWarning_EmitsWarning
is asserting a Mode-3 warning that should be ignored; update the assertions to
reflect Mode 3 behaviour by removing the expectation of a subset validation
warning and asserting no such diagnostic is emitted. Specifically, change the
checks after config load: remove or replace
Collector.Warnings.Should().BeGreaterThan(0) with an assertion that no warnings
are emitted (e.g., Collector.Warnings.Should().Be(0) or equivalent) and change
Collector.Diagnostics.Should().Contain(d => d.Message.Contains("Context
'security' includes products [security]")) to
Collector.Diagnostics.Should().NotContain(...), so the test aligns with
ChangelogConfigurationLoader's Mode 3 logic for rules.bundle.products.
---
Nitpick comments:
In `@src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs`:
- Around line 714-785: The ruleStats dictionary is being used both for numeric
counters and a boolean flag ("ineffective_pattern_warned") which yields
confusing summary output; change the implementation in the
ChangelogBundlingService loop to keep ruleStats strictly numeric and introduce a
separate boolean (e.g., ineffectivePatternWarned) used by the per-product branch
(where resolveResult.Rule.MatchProducts is checked) to ensure the
collector.EmitHint for the ineffective-pattern warning is emitted only once;
update the final summary assembly (where ruleStats is read and the message
variable is built) to not include the boolean, and ensure references to
ruleStats and the new ineffectivePatternWarned flag are used in
ResolvePerProductBundleRule/ShouldExcludeByResolvedProductRule handling blocks
as needed.
In `@tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs`:
- Around line 1171-1193: The test method
LoadChangelogConfiguration_WithRulesBundle_MatchProductsConjunction_LoadsCorrectly
is a public async Task and must be renamed to include the Async suffix (e.g.,
LoadChangelogConfiguration_WithRulesBundle_MatchProductsConjunction_LoadsCorrectlyAsync);
update the method name and any references to it (test discovery uses the method
name) and apply the same renaming pattern to the other public async tests in
this file (the ones noted in the comment range) so all public async methods
follow PascalCase + Async convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 829385b9-6ec9-4aa8-8c25-6c6ece3936f1
📒 Files selected for processing (16)
config/changelog.example.ymldocs/cli/release/changelog-bundle.mddocs/cli/release/changelog-gh-release.mddocs/contribute/changelog.mdsrc/Elastic.Documentation.Configuration/Changelog/BlockConfiguration.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cssrc/Elastic.Documentation/ReleaseNotes/PublishBlocker.cssrc/Elastic.Documentation/ReleaseNotes/PublishBlockerExtensions.cssrc/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cssrc/services/Elastic.Changelog/Bundling/ProfileFilterResolver.cssrc/services/Elastic.Changelog/Configuration/ChangelogConfigurationLoader.cssrc/services/Elastic.Changelog/Creation/PrInfoProcessor.cstests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleRulesExtensionsTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cstests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/PublishBlockerExtensionsTests.cs
✅ Files skipped from review due to trivial changes (1)
- docs/cli/release/changelog-gh-release.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/cli/release/changelog-bundle.md
Comment on lines
+341
to
+350
| var parts = entry.Split(' ', StringSplitOptions.RemoveEmptyEntries); | ||
| if (parts.Length < 1) | ||
| continue; // Skip empty entries | ||
|
|
||
| return | ||
| [ | ||
| new ProductArgument | ||
| products.Add(new ProductArgument | ||
| { | ||
| Product = productId == "*" ? "*" : productId, | ||
| Target = target == "*" ? "*" : target, | ||
| Lifecycle = lifecycle == "*" ? "*" : lifecycle | ||
| } | ||
| ]; | ||
| Product = parts.Length > 0 ? (parts[0] == "*" ? "*" : parts[0]) : "*", | ||
| Target = parts.Length > 1 ? (parts[1] == "*" ? "*" : parts[1]) : "*", | ||
| Lifecycle = parts.Length > 2 ? (parts[2] == "*" ? "*" : parts[2]) : "*" | ||
| }); |
There was a problem hiding this comment.
Reject malformed product entries instead of silently truncating.
If an entry has more than 3 tokens, extra tokens are ignored today. A missing comma can therefore be misparsed into a wrong filter context without any error, leading to incorrect changelog inclusion/exclusion.
Proposed fix
foreach (var entry in productEntries)
{
var parts = entry.Split(' ', StringSplitOptions.RemoveEmptyEntries);
- if (parts.Length < 1)
- continue; // Skip empty entries
+ if (parts.Length is < 1 or > 3)
+ continue; // Skip malformed entries
products.Add(new ProductArgument
{
- Product = parts.Length > 0 ? (parts[0] == "*" ? "*" : parts[0]) : "*",
- Target = parts.Length > 1 ? (parts[1] == "*" ? "*" : parts[1]) : "*",
- Lifecycle = parts.Length > 2 ? (parts[2] == "*" ? "*" : parts[2]) : "*"
+ Product = parts[0],
+ Target = parts.Length > 1 ? parts[1] : "*",
+ Lifecycle = parts.Length > 2 ? parts[2] : "*"
});
}As per coding guidelines, "Fail fast by throwing exceptions early rather than hiding errors".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var parts = entry.Split(' ', StringSplitOptions.RemoveEmptyEntries); | |
| if (parts.Length < 1) | |
| continue; // Skip empty entries | |
| return | |
| [ | |
| new ProductArgument | |
| products.Add(new ProductArgument | |
| { | |
| Product = productId == "*" ? "*" : productId, | |
| Target = target == "*" ? "*" : target, | |
| Lifecycle = lifecycle == "*" ? "*" : lifecycle | |
| } | |
| ]; | |
| Product = parts.Length > 0 ? (parts[0] == "*" ? "*" : parts[0]) : "*", | |
| Target = parts.Length > 1 ? (parts[1] == "*" ? "*" : parts[1]) : "*", | |
| Lifecycle = parts.Length > 2 ? (parts[2] == "*" ? "*" : parts[2]) : "*" | |
| }); | |
| var parts = entry.Split(' ', StringSplitOptions.RemoveEmptyEntries); | |
| if (parts.Length is < 1 or > 3) | |
| continue; // Skip malformed entries | |
| products.Add(new ProductArgument | |
| { | |
| Product = parts[0], | |
| Target = parts.Length > 1 ? parts[1] : "*", | |
| Lifecycle = parts.Length > 2 ? parts[2] : "*" | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/Elastic.Changelog/Bundling/ProfileFilterResolver.cs` around
lines 341 - 350, The parser currently silently ignores extra tokens in entries;
update the logic that builds ProductArgument (the code handling variable parts
and the products.Add call) to validate token count: after splitting into parts,
if parts.Length is 0 continue, if parts.Length > 3 throw a descriptive
FormatException/ArgumentException that includes the offending entry string, and
only then create ProductArgument using parts[0..2] with default "*" for missing
tokens; this enforces fail-fast behavior for malformed product entries in the
ProductArgument creation path.
Comment on lines
+4366
to
+4369
| // Assert - elasticsearch entry is excluded by exclude_products rule even with InputProducts | ||
| result.Should().BeFalse("Bundle should fail because all entries are excluded by rules.bundle"); | ||
| Collector.Diagnostics.Should().Contain(d => d.Message.Contains("[-bundle-exclude]") && d.Message.Contains("1755268130-elasticsearch-feature.yaml")); | ||
| Collector.Errors.Should().BeGreaterThan(0, "Should have error about no entries remaining"); |
There was a problem hiding this comment.
Use the current product-filter diagnostic tag.
This assertion is still looking for [-bundle-exclude], but the bundle service emits [-bundle-global] for this global product-filter path. As written, the test will fail even if the exclusion is working correctly.
Suggested fix
- Collector.Diagnostics.Should().Contain(d => d.Message.Contains("[-bundle-exclude]") && d.Message.Contains("1755268130-elasticsearch-feature.yaml"));
+ Collector.Diagnostics.Should().Contain(d => d.Message.Contains("[-bundle-global]") && d.Message.Contains("1755268130-elasticsearch-feature.yaml"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cs` around
lines 4366 - 4369, The assertion is checking for the wrong diagnostic tag;
update the diagnostic check in the test (the block that asserts
Collector.Diagnostics contains a message) to look for "[-bundle-global]" instead
of "[-bundle-exclude]" so it matches the bundle service's global product-filter
diagnostic; keep the rest of the predicate (e.g., checking for
"1755268130-elasticsearch-feature.yaml") and leave the surrounding assertions on
result and Collector.Errors unchanged.
Comment on lines
+5053
to
+5104
| public async Task BundleChangelogs_WithPerProductRules_FallsBackToGlobalWhenNoContextRule() | ||
| { | ||
| // language=yaml | ||
| var configContent = | ||
| """ | ||
| rules: | ||
| bundle: | ||
| include_products: | ||
| - elasticsearch | ||
| - security | ||
| products: | ||
| cloud-hosted: | ||
| include_products: | ||
| - security | ||
| - kibana | ||
| """; | ||
|
|
||
| var configPath = FileSystem.Path.Combine(FileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "changelog.yml"); | ||
| FileSystem.Directory.CreateDirectory(FileSystem.Path.GetDirectoryName(configPath)!); | ||
| await FileSystem.File.WriteAllTextAsync(configPath, configContent, TestContext.Current.CancellationToken); | ||
|
|
||
| // Create test entries | ||
| var changelogDir = CreateChangelogDir(); | ||
| await CreateTestEntry(changelogDir, "kibana-feature.yaml", "Kibana feature", "kibana"); | ||
| await CreateTestEntry(changelogDir, "security-feature.yaml", "Security feature", "security"); | ||
| await CreateTestEntry(changelogDir, "elasticsearch-feature.yaml", "Elasticsearch feature", "elasticsearch"); | ||
|
|
||
| var outputPath = CreateTempFilePath("bundle.yaml"); | ||
|
|
||
| var input = new BundleChangelogsArguments | ||
| { | ||
| Directory = changelogDir, | ||
| All = true, | ||
| Config = configPath, | ||
| Output = outputPath, | ||
| OutputProducts = [new ProductArgument { Product = "security", Target = "9.3.0" }] // No context rule for security, should fall back to global | ||
| }; | ||
|
|
||
| // Act | ||
| var result = await ServiceWithConfig.BundleChangelogs(Collector, input, TestContext.Current.CancellationToken); | ||
|
|
||
| // Assert | ||
| // Rule context = "security" (from output products) | ||
| // No per-product rule for "security" → falls back to global rules | ||
| // elasticsearch and kibana entries are disjoint from security context → excluded | ||
| // Only security entry remains and uses global rules (include elasticsearch, security) → included | ||
| result.Should().BeTrue($"Errors: {string.Join("; ", Collector.Diagnostics.Select(d => d.Message))}"); | ||
| var bundleContent = await FileSystem.File.ReadAllTextAsync(outputPath, TestContext.Current.CancellationToken); | ||
| bundleContent.Should().Contain("security-feature.yaml", "security entry should be included by global rule"); | ||
| // Disjoint entries are excluded entirely in single-product rule resolution | ||
| bundleContent.Should().NotContain("elasticsearch-feature.yaml", "elasticsearch entry is disjoint from security context"); | ||
| bundleContent.Should().NotContain("kibana-feature.yaml", "kibana entry is disjoint from security context"); |
There was a problem hiding this comment.
This still passes under the old fallback behavior.
security-feature.yaml is included both when a missing per-product block correctly passes through and when globals are incorrectly reused, so this test does not actually lock in the new Mode 3 behavior.
Suggested tweak
rules:
bundle:
- include_products:
- - elasticsearch
- - security
+ exclude_types:
+ - feature
products:
cloud-hosted:
include_products:
- security
- kibanaWith that change, security-feature.yaml should still be included only if missing-context rules truly bypass the global filters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Elastic.Changelog.Tests/Changelogs/BundleChangelogsTests.cs` around
lines 5053 - 5104, The test
BundleChangelogs_WithPerProductRules_FallsBackToGlobalWhenNoContextRule
currently passes under both old and new fallback behaviors because globals
include "security"; update the test so globals do NOT include "security" (e.g.,
change the top-level rules.bundle.include_products to only "elasticsearch") and
keep OutputProducts = [new ProductArgument { Product = "security", Target =
"9.3.0" }]; this way security-feature.yaml will only be included if
missing-context per-product rules are correctly bypassing global filters; adjust
the assertions to expect security-feature.yaml included and confirm that
globals-excluded entries remain excluded using
ServiceWithConfig.BundleChangelogs and BundleChangelogsArguments/ProductArgument
references.
Comment on lines
+1518
to
+1549
| [Fact] | ||
| public async Task LoadChangelogConfiguration_WithPerProductProductFiltering_SubsetValidationWarning_EmitsWarning() | ||
| { | ||
| // Arrange | ||
| var configLoader = new ChangelogConfigurationLoader(LoggerFactory, ConfigurationContext, FileSystem); | ||
| var configPath = FileSystem.Path.Combine(FileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "changelog.yml"); | ||
| FileSystem.Directory.CreateDirectory(FileSystem.Path.GetDirectoryName(configPath)!); | ||
|
|
||
| var configContent = | ||
| """ | ||
| rules: | ||
| bundle: | ||
| include_products: | ||
| - elasticsearch | ||
| - kibana | ||
| products: | ||
| security: | ||
| include_products: | ||
| - security | ||
| - elasticsearch | ||
| - kibana | ||
| """; | ||
| await FileSystem.File.WriteAllTextAsync(configPath, configContent, TestContext.Current.CancellationToken); | ||
|
|
||
| // Act | ||
| var config = await configLoader.LoadChangelogConfiguration(Collector, configPath, TestContext.Current.CancellationToken); | ||
|
|
||
| // Assert | ||
| config.Should().NotBeNull(); | ||
| Collector.Warnings.Should().BeGreaterThan(0); | ||
| Collector.Diagnostics.Should().Contain(d => d.Message.Contains("Context 'security' includes products [security]")); | ||
| } |
There was a problem hiding this comment.
This test locks in a Mode 3 warning that the new contract says should not matter.
Mode 3 explicitly ignores bundle-level product filters once rules.bundle.products is populated. Expecting a warning because security.include_products is not a subset of the bundle-level include_products reintroduces that coupling and conflicts with the three-mode behavior described in the PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Elastic.Changelog.Tests/Changelogs/ChangelogConfigurationTests.cs`
around lines 1518 - 1549, The test
LoadChangelogConfiguration_WithPerProductProductFiltering_SubsetValidationWarning_EmitsWarning
is asserting a Mode-3 warning that should be ignored; update the assertions to
reflect Mode 3 behaviour by removing the expectation of a subset validation
warning and asserting no such diagnostic is emitted. Specifically, change the
checks after config load: remove or replace
Collector.Warnings.Should().BeGreaterThan(0) with an assertion that no warnings
are emitted (e.g., Collector.Warnings.Should().Be(0) or equivalent) and change
Collector.Diagnostics.Should().Contain(d => d.Message.Contains("Context
'security' includes products [security]")) to
Collector.Diagnostics.Should().NotContain(...), so the test aligns with
ChangelogConfigurationLoader's Mode 3 logic for rules.bundle.products.
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.
Problem
While testing elastic/kibana#258941 and elastic/docs-content#5582 it became obvious that some changelogs were appearing in docs where they weren't expected.
For example,
242105.yamlwas picked up in the command "docs-builder changelog bundle observability-release 9.3.0 ./docs/9.3.0-observability.txt` when IMO it shouldn't have been.The changelog configuration file contains:
The relevant changelog does not have either a
productsnorareasthat matches theobservabilitycontext (as indicated by theoutput_products). I tried removing{version}from theoutput_productsand that didn't affect it either. It was unclear why the "changelog bundle" command thought this changelog matched the rules.Solution summary
What problem this solves
Earlier behavior mixed global
rules.bundlefilters with per-product maps in ways that were hard to reason about (global vs per-product precedence, disjoint multi-product changelogs, OR-styleinclude_products, empty/missingproducts). This PR:rules.bundleend-to-end.match_products/match_areas(and related)conjunctionsemantics where every configured ID must appear on the changelog for the include/exclude rule to match as documented.Final behavior: three bundle rule modes
Mode 1 — No filtering
No effective
rules.bundle(nothing to apply). No product / type / area filtering fromrules.bundle.Mode 2 — Global content
Global-only
rules.bundle(productsabsent or empty). Rules use each changelog’s ownproducts,type, andareas. No single rule-context product and no disjoint exclusion.include_products/exclude_productswithmatch_productsany/all/conjunctionapply to changelog content as documented.products: included with a warning where applicable; type/area rules still apply when configured.Mode 3 — Per-product context
Non-empty
rules.bundle.products. Globalrules.bundleproduct/type/area fields are not used for filtering. Resolution uses a single rule-context product; disjoint changelogs are excluded; empty/missing products are excluded with a warning; if there’s no per-product block for the context product, entries pass through (no global fallback).Conjunction matching (
conjunction)MatchMode.Conjunctionis added/used for multi-valued product and area matching (and wired through config loading, bundling, publish blockers, and PR create label rules whereMatchModeapplies).include_*, every listed ID must appear on the changelog to match; forexclude_*, every listed ID must appear to be excluded — see the tables indocs/contribute/changelog.mdfor precise semantics.Code changes (high level)
BundleFilterMode,BundleRules.DetermineFilterMode()(or equivalent extension) to classify config.ResolveResult/ResolveResultWithRuleincludingPassThroughfor Mode 3.ChangelogBundlingService: after primary gather (--all,--prs,--issues,--input-products, …), applyrules.bundleby mode; product matching includesConjunction.ChangelogConfigurationLoader: parseconjunction, adjust hints/warnings (e.g. Mode 3 ignores globals).ProfileFilterResolver: alignment with bundle/product handling.PrInfoProcessor:MatchMode.Conjunctionfor label rules.PublishBlockerExtensions+MatchMode: conjunction for area matching.Docs and examples
docs/contribute/changelog.md: bundle rule modes, Mode 2 vs 3, single-product resolution, conjunction tables for products and areas.docs/cli/release/changelog-bundle.md&changelog-gh-release.md: aligned with modes;--input-productsvsrules.bundle(gather vs filter); profile vs option-based.config/changelog.example.yml: trimmed inline duplication; points to docs for rules detail.Tests
BundleChangelogsTests: large expansion (global vs per-product, conjunction, empty products, regressions).BundleRulesExtensionsTests: mode detection.ChangelogConfigurationTests: loader/config cases.PublishBlockerExtensionsTests: area conjunction (and related).Steps to test
I re-tested Kibana scenarios as follows:
rules.bundlecombinations. For example, start with allrules.bundledetails removed/commented out.release_note:skiplabel):Warning: No changelog file found for PR: https://github.com/elastic/kibana/pull/233289product: elasticsearchandproduct: observabilityeven though the bundle'soutput_productsiskibana. This is working as expected.product: observability. The messages here are expected (since some changelogs are now being filtered out of the bundle):Warning: [-bundle-include] Excluding '243409.yaml' from bundle (global product filter). NOTE: docs/changelog/243409.yaml 0 Errors / 38 Warnings / 0 Hintsexclude_areas: "AI for observability"then239144.yamlis also excluded (it had been included before that since it had both kibana and observability products and multiple areas).conjunctionfunctionality. For example:kibanaandobservabilityin their products list (as opposed toallwhere changelogs that had only one or the other would be included).output_productswaskibana(and therefore that's the bundle'sproductslist) any changelogs that don't have intersecting products (e.g. the changelogs that only haveelasticsearchproduct) are excluded. The bundle still contains changelogs that have bothkibanaandobservabilityproducts as long as they don't have any of the excluded areas.observabilityproducts. The changelogs that have bothkibanaandsecurityproducts persist.I also tested in a private repo with a combination of changelogs that had
product: elasticsearchandproduct: cloud-serverless. The results were as expected.Outstanding tasks
Generative AI disclosure
Tool(s) and model(s) used: composer-2, claude-4-sonnet