[Release notes] Add bundle-amend --remove option#3513
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds the ability to remove changelog entries from published bundles via a new Sequence DiagramsequenceDiagram
rect rgba(100, 149, 237, 0.5)
note over CLI,FS: bundle-amend --remove flow
end
participant CLI as docs-builder CLI
participant ChangelogCommand
participant AmendService as ChangelogBundleAmendService
participant Merger as BundleAmendMerger
participant FS as FileSystem / YAML
CLI->>ChangelogCommand: bundle-amend --remove file.yaml [--force] [--dry-run]
ChangelogCommand->>ChangelogCommand: normalize paths, validate at least one of --add/--remove
ChangelogCommand->>AmendService: AmendBundle(AmendBundleArguments)
AmendService->>FS: deserialize parent bundle + existing amend bundles
AmendService->>Merger: CollectAppliedExclusionKeys(existingAmends)
loop each RemoveFile
AmendService->>FS: read file, compute checksum
AmendService->>Merger: EntryMatchesExclusion(entry, exclusion)
AmendService->>Merger: BuildExclusionKey(exclusion)
AmendService-->>AmendService: BuildExclusionEntryAsync → Add | Skip | fail
end
AmendService->>Merger: MergeEntries(parentEntries, existingAmends)
alt --dry-run
AmendService-->>CLI: return success (no write)
else write amend
AmendService->>FS: write .amend-N.yaml with exclude-entries + entries
AmendService-->>CLI: return success
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/syntax/changelog.md (1)
322-326:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't recommend
--removeas a fix for an already-missing file.The PR notes this path still fails once the changelog file is no longer on disk. For that case, tell readers to restore the file or re-bundle with
--resolve;bundle-amend --removeonly applies while the source changelog is still available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/syntax/changelog.md` around lines 322 - 326, The documentation in the changelog.md file currently recommends using `bundle-amend --remove` as a general fix for missing changelog files, but this option only works when the source changelog file is still available on disk. Remove the `--remove` option from the list of fixes in the bullet points, or alternatively, add a note clarifying that this option only applies when the source file exists. Keep only the two viable fixes for missing files: restoring the missing changelog files or re-creating the bundle with `--resolve` to embed entry content directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs`:
- Around line 281-313: Orphan amend files that don't have a corresponding parent
bundle are being included in the final bundle output as standalone bundles
because they are not added to mergedAmendPaths when the parent lookup fails.
When the bundlesByPath.TryGetValue check fails for parentPath and the continue
statement executes at line 288, you must still add all amend files in the
current group to mergedAmendPaths before continuing, so that these orphan amend
files are properly excluded from the final returned bundle list.
In `@src/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cs`:
- Around line 341-349: In the LoadExistingAmendBundlesAsync method, the call to
ReleaseNotesSerialization.DeserializeBundle on line 348 lacks error handling for
malformed YAML. Wrap the ReadAllTextAsync and DeserializeBundle calls in a
try-catch block to catch deserialization exceptions, log the error for
diagnostics purposes, and continue processing the remaining amend files instead
of allowing the exception to propagate and crash the service.
In `@src/services/Elastic.Changelog/Bundling/ChangelogRemoveService.cs`:
- Around line 360-366: The deserialization of individual amend files using
ReleaseNotesSerialization.DeserializeBundle at lines 360-366 is not protected,
so if one amend file fails to deserialize, the exception propagates to the outer
catch block and skips dependency analysis for the entire parent bundle,
potentially allowing deletion of required files. Wrap the
ReleaseNotesSerialization.DeserializeBundle call in a try-catch block within the
loop over amendPaths so that individual deserialization failures are handled
gracefully without preventing the parent bundle's dependency analysis. Only add
valid deserialized bundles to amendBundles and continue processing remaining
amend files. Apply the same fix to the second location mentioned at lines
394-397.
---
Outside diff comments:
In `@docs/syntax/changelog.md`:
- Around line 322-326: The documentation in the changelog.md file currently
recommends using `bundle-amend --remove` as a general fix for missing changelog
files, but this option only works when the source changelog file is still
available on disk. Remove the `--remove` option from the list of fixes in the
bullet points, or alternatively, add a note clarifying that this option only
applies when the source file exists. Keep only the two viable fixes for missing
files: restoring the missing changelog files or re-creating the bundle with
`--resolve` to embed entry content directly.
🪄 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: Enterprise
Run ID: 794f960c-9f1a-4c58-8bf0-efb55995e9c3
📒 Files selected for processing (19)
docs/cli-schema.jsondocs/cli/changelog/cmd-bundle-amend.mddocs/contribute/bundle-changelogs.mddocs/syntax/changelog.mdsrc/Elastic.Documentation.Configuration/ReleaseNotes/Bundle.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleAmendMerger.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/ReleaseNotesSerialization.cssrc/Elastic.Documentation/ReleaseNotes/Bundle.cssrc/services/Elastic.Changelog/Bundling/ChangelogBundleAmendService.cssrc/services/Elastic.Changelog/Bundling/ChangelogRemoveService.cssrc/services/Elastic.Changelog/Rendering/BundleValidationService.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Changelogs/BundleAmendMergerTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleAmendTests.cstests/Elastic.Changelog.Tests/Changelogs/BundleLoading/BundleLoaderTests.cstests/Elastic.Changelog.Tests/Changelogs/ChangelogRemoveTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/BundleValidationTests.cstests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
Good feature overall — the BundleAmendMerger extraction is clean and the test coverage is solid. Two issues worth fixing before merge.
lcawl
had a problem deploying
to
integration-tests
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs`:
- Line 97: The call to MergeAmendFiles on line 97 in the LoadBundlesFromContent
method is missing a required parameter, causing a compile error due to arity
mismatch. Add the missing emitWarning parameter to the MergeAmendFiles method
call to match its expected signature. Determine the appropriate value for the
emitWarning parameter based on the context of how this method should behave and
the overall design of the LoadBundlesFromContent method.
🪄 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: Enterprise
Run ID: 49523e79-88c4-426d-bd01-3c5c01418a61
📒 Files selected for processing (8)
docs/cli-schema.jsondocs/cli/changelog/cmd-remove.mddocs/syntax/changelog.mdsrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cssrc/services/Elastic.Changelog/Rendering/BundleValidationService.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Changelogs/BundleLoading/BundleLoaderTests.cstests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
- src/tooling/docs-builder/Commands/ChangelogCommand.cs
- tests/Elastic.Changelog.Tests/Changelogs/BundleLoading/BundleLoaderTests.cs
- docs/cli-schema.json
- src/services/Elastic.Changelog/Rendering/BundleValidationService.cs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs`:
- Line 97: The call to MergeAmendFiles on line 97 in the LoadBundlesFromContent
method is missing a required parameter, causing a compile error due to arity
mismatch. Add the missing emitWarning parameter to the MergeAmendFiles method
call to match its expected signature. Determine the appropriate value for the
emitWarning parameter based on the context of how this method should behave and
the overall design of the LoadBundlesFromContent method.
🪄 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: Enterprise
Run ID: 49523e79-88c4-426d-bd01-3c5c01418a61
📒 Files selected for processing (8)
docs/cli-schema.jsondocs/cli/changelog/cmd-remove.mddocs/syntax/changelog.mdsrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cssrc/services/Elastic.Changelog/Rendering/BundleValidationService.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Changelogs/BundleLoading/BundleLoaderTests.cstests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
- src/tooling/docs-builder/Commands/ChangelogCommand.cs
- tests/Elastic.Changelog.Tests/Changelogs/BundleLoading/BundleLoaderTests.cs
- docs/cli-schema.json
- src/services/Elastic.Changelog/Rendering/BundleValidationService.cs
🛑 Comments failed to post (1)
src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs (1)
97-97:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix method call arity mismatch in
LoadBundlesFromContent.Line 97 calls
MergeAmendFileswithout the requiredemitWarningparameter, which causes a compile error.Suggested fix
- return MergeAmendFiles(loadedBundles); + return MergeAmendFiles(loadedBundles, emitWarning);📝 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.return MergeAmendFiles(loadedBundles, emitWarning);🧰 Tools
🪛 GitHub Check: integration
[failure] 97-97:
There is no argument given that corresponds to the required parameter 'emitWarning' of 'BundleLoader.MergeAmendFiles(List, Action)'
[failure] 97-97:
There is no argument given that corresponds to the required parameter 'emitWarning' of 'BundleLoader.MergeAmendFiles(List, Action)'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cs` at line 97, The call to MergeAmendFiles on line 97 in the LoadBundlesFromContent method is missing a required parameter, causing a compile error due to arity mismatch. Add the missing emitWarning parameter to the MergeAmendFiles method call to match its expected signature. Determine the appropriate value for the emitWarning parameter based on the context of how this method should behave and the overall design of the LoadBundlesFromContent method.Source: Linters/SAST tools
2 participants
Summary
Add a
--removeoption to thedocs-builder changelog bundle-amendcommand, so that changelogs can be removed from bundles post-release if necessary.Previously, the bundle had to be manually edited or entirely replaced.
Core changes
Schema — Added
exclude-entriestoBundle.cs,BundleDto, andReleaseNotesSerialization.cs.BundleAmendMerger— New shared merger inBundleAmendMerger.cs: per amend file, apply exclusions then additions; ordered by amend number.Merge wiring — Updated
BundleLoader.csandBundleValidationService.csto use the merger.ChangelogBundleAmendService— Supports--remove,--force, and--dry-run; writesexclude-entriesto new.amend-N.yamlsidecars; checksum matching with optional--forcefor name-only removal.CLI —
ChangelogCommand.BundleAmendaccepts--remove,--force,--dry-run; requires at least one of--addor--remove.changelog remove—ChangelogRemoveServiceuses the effective bundle (parent + amends − exclusions) for dependency checks; skips amend files as parent bundle candidates.Tests added
BundleAmendMergerTests.csBundleAmendTests.csBundleLoaderTests.csChangelogRemoveTests.cschangelog renderomits excluded entry inBundleValidationTests.cs{changelog}directive omits excluded entry and merged bundle has one entry inChangelogBasicTests.csExample usage
Steps for testing
I tested this command as follows:
changelog bundle-amendcommand to remove a changelog. For example, I tested against a bundle with a single changelog in the aforementioned repo:Error: Bundle contains 'upstream-update.yaml' but with a different checksum than the file on disk. Re-create the bundle or use --force to remove by file name only. NOTE: /path/elastic-otel-java/docs/changelog/upstream-update.yaml--forceoption, it successfully created adocs/releases/1.11.0.amend-1.yamlfile with the following contents:NOTE: If you try to remove a changelog from a bundle but you don't have access to the changelog file anymore, you get an error like this:
Generative AI disclosure
Tool(s) and model(s) used: composer-2.5-fast