changelogs: Add --plan flag to changelog bundle#3028
Conversation
coderabbitai
bot
added
documentation
|
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:
📝 WalkthroughWalkthroughAdds a Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Cmd as ChangelogCommand
participant Svc as ChangelogBundlingService
participant Config as Configuration Loader
participant GH as GitHub Actions Outputs
User->>Cmd: Run `changelog bundle --plan`
activate Cmd
Cmd->>Svc: PlanBundleAsync(collector, args, hasReleaseVersion, ctx)
activate Svc
Svc->>Config: Load changelog configuration (profile or standard) if needed
activate Config
Config-->>Svc: Return configuration / profile metadata
deactivate Config
Svc->>Svc: Evaluate NeedsNetwork / NeedsGithubToken (e.g., github_release)
Svc->>Svc: Compute OutputPath (profile templates or defaults)
Svc-->>Cmd: Return BundlePlanResult{NeedsNetwork, NeedsGithubToken, OutputPath}
deactivate Svc
Cmd->>GH: Set outputs (needs_network, needs_github_token, output_path) or write to stdout
Cmd-->>User: Exit (no bundle created)
deactivate Cmd
🚥 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.
🧹 Nitpick comments (1)
tests/Elastic.Changelog.Tests/Changelogs/BundlePlanTests.cs (1)
161-185: Misleading test name.The test is named
Plan_ProfileNotFound_ReturnsNullbut assertsresult.Should().NotBeNull(). Consider renaming toPlan_ProfileNotFound_ReturnsResultWithNoNetworkRequirementto match the actual behavior being tested.Suggested rename
[Fact] -public async Task Plan_ProfileNotFound_ReturnsNull() +public async Task Plan_ProfileNotFound_ReturnsResultWithNoNetworkRequirement()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Elastic.Changelog.Tests/Changelogs/BundlePlanTests.cs` around lines 161 - 185, Rename the misleading test method Plan_ProfileNotFound_ReturnsNull to reflect its assertions (e.g., Plan_ProfileNotFound_ReturnsResultWithNoNetworkRequirement); update the method name declaration in the BundlePlanTests class and any references to it so the test name matches the behavior being asserted (result is not null and NeedsNetwork is false) when calling Service.PlanBundleAsync with a nonexistent profile via the BundleChangelogsArguments instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Elastic.Changelog.Tests/Changelogs/BundlePlanTests.cs`:
- Around line 161-185: Rename the misleading test method
Plan_ProfileNotFound_ReturnsNull to reflect its assertions (e.g.,
Plan_ProfileNotFound_ReturnsResultWithNoNetworkRequirement); update the method
name declaration in the BundlePlanTests class and any references to it so the
test name matches the behavior being asserted (result is not null and
NeedsNetwork is false) when calling Service.PlanBundleAsync with a nonexistent
profile via the BundleChangelogsArguments instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b7a7b5c-1c2b-4521-b5f4-5f5005869e03
📒 Files selected for processing (4)
docs/cli/changelog/bundle.mdsrc/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Changelogs/BundlePlanTests.cs
coderabbitai
bot
added
feature
and removed
documentation
coderabbitai
bot
added
documentation
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/tooling/docs-builder/Commands/ChangelogCommand.cs`:
- Around line 506-507: The XML documentation for the Bundle method mentions
parameters sanitizePrivateLinks and noSanitizePrivateLinks that don't exist;
update the Bundle method signature or the docs so they match: either add bool?
sanitizePrivateLinks and bool? noSanitizePrivateLinks parameters to the Bundle
method (and propagate their use to bundle-time sanitization logic and any
callers), or remove the two <param> XML entries from the Bundle method's
docblock so the docs reflect the actual signature; locate the Bundle method and
its XML comments by the method name Bundle to apply the fix.
🪄 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: 57f3dc19-7cad-4c86-901f-6969fb23aaed
📒 Files selected for processing (3)
docs/cli/changelog/bundle.mdsrc/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cs
coderabbitai
bot
added
feature
and removed
documentation
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/tooling/docs-builder/Commands/ChangelogCommand.cs`:
- Around line 505-506: The XML doc for the "plan" parameter is incorrect: it
says "Output a structured JSON plan" but the implementation in ChangelogCommand
uses SetOutputAsync to emit GitHub Actions outputs (e.g., "needs_network",
"needs_github_token", "output_path"); update the <param name="plan"> summary to
describe that the command emits GitHub Actions outputs (names of outputs and
purpose) and exits without generating the bundle, or alternately note that it
writes a structured JSON to an output variable only if behavior changes;
reference the ChangelogCommand implementation and SetOutputAsync and the output
keys ("needs_network", "needs_github_token", "output_path") when editing the XML
doc.
🪄 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: 37c9fda3-2e15-4413-998e-7fc1e757caf4
📒 Files selected for processing (1)
src/tooling/docs-builder/Commands/ChangelogCommand.cs
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
Review (nitpicks + small fixes)
Thanks for the --plan flow — this matches what we need for docs-actions and avoids duplicating resolution logic in JS.
1. Use filesystem path joining in PlanBundleAsync (not UrlPath.Join)
UrlPath is for URL-style paths (always /). The plan output output_path is a filesystem path for CI/Docker; it should match how the real bundle path is built elsewhere (_fileSystem.Path.Join / Path.Join in ApplyConfigDefaults and BundleChangelogs).
Suggestion: replace UrlPath.Join with _fileSystem.Path.Join (or Path.Join consistent with ApplyConfigDefaults line ~432) and drop the comment about “CI runners expect forward slashes” — Windows runners need correct OS separators.
// Resolve output path — mirrors the logic in ProcessProfile + ApplyConfigDefaults.
var outputPath = input.Output;
if (string.IsNullOrWhiteSpace(outputPath) && profileDef?.Output != null)
{
var version = input.ProfileArgument ?? "unknown";
var lifecycle = VersionLifecycleInference.InferLifecycle(version);
var outputPattern = profileDef.Output
.Replace("{version}", version)
.Replace("{lifecycle}", lifecycle);
var outputDir = config?.Bundle?.OutputDirectory
?? config?.Bundle?.Directory
?? _fileSystem.Directory.GetCurrentDirectory();
outputPath = _fileSystem.Path.Join(outputDir, outputPattern);
}
else if (string.IsNullOrWhiteSpace(outputPath) && config?.Bundle?.OutputDirectory != null)
outputPath = Path.Join(config.Bundle.OutputDirectory, "changelog-bundle.yaml");(Adjust if you prefer Path.Join for both branches to match ApplyConfigDefaults exactly.)
2. XML / docs: say “GitHub Actions outputs”, not JSON
PlanBundleAsync<summary>currently says CI consumes “structured JSON”; implementation sets GHA step outputs viaSetOutputAsync(needs_network,needs_github_token,output_path). Worth aligning the summary with that.- The
planparameter onBundlehas the same “JSON plan” wording — suggest describing the output keys and that no bundle is written.
3. Test name vs assertions
Plan_ProfileNotFound_ReturnsNull asserts result.Should().NotBeNull(). Consider renaming to something like Plan_ProfileNotFound_StillReturnsResult_WithNeedsNetworkFalse (or similar) so the name matches behavior.
Happy to re-review after these tweaks.
|
Clarification on the review: for joining paths in |
src/services/Elastic.Changelog/Bundling/ChangelogBundlingService.cs
Outdated
Show resolved
Hide resolved
* Add --plan flag to bundle * Update bundle docs * use UrlPath * Fix merge mishap * Add allowlist and https checks for report URLs * Use ScopedFileSystem, update xml docs * Adjust test names * Fix tests on Windows * Sanitize paths * Use OptionalWindowsReplace
3 participants
Dependency of elastic/docs-actions#42
This pull request adds a new
--planmode to the changelog bundling CLI and supporting code, allowing CI actions to determine required Docker flags, network access, and output paths before actually generating the bundle. The implementation includes a new planning method, structured result type, CLI wiring, and comprehensive tests.The most important changes are:
New Planning Feature:
--planoption to the CLI, documented indocs/cli/changelog/bundle.md, which outputs a structured set of CI step outputs describing Docker flags, network requirements, and the resolved output path, then exits without generating the bundle.BundlePlanResultrecord type to represent the plan output, and implemented thePlanBundleAsyncmethod inChangelogBundlingServiceto resolve plan details from config/profile metadata without executing network calls or file scanning. [1] [2]CLI and Service Integration:
Bundlecommand inChangelogCommand.csto support the--planflag, wiring it to call the new planning method and emit outputs for CI consumption. [1] [2] [3] [4] [5]Testing:
BundlePlanTeststo verify plan output logic for various config and profile scenarios, including output path resolution, network requirements, and error handling.