Add changelog bundle and render commands #2341
Conversation
🔍 Preview links for changed docs |
![github-code-quality[bot]](/s/com/githubusercontent/avatars/G.https/u/9919?s=60&v=4){kind=small}
lcawl
changed the title
lcawl
changed the title
| // Validate required fields in resolved entry | ||
| if (string.IsNullOrWhiteSpace(entry.Title)) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, $"Entry in bundle is missing required field: title"); | ||
| return false; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(entry.Type)) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, $"Entry in bundle is missing required field: type"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
| // Validate required fields in resolved entry | |
| if (string.IsNullOrWhiteSpace(entry.Title)) | |
| { | |
| collector.EmitError(bundleInput.BundleFile, $"Entry in bundle is missing required field: title"); | |
| return false; | |
| } | |
| if (string.IsNullOrWhiteSpace(entry.Type)) | |
| { | |
| collector.EmitError(bundleInput.BundleFile, $"Entry in bundle is missing required field: type"); | |
| return false; | |
| } |
None of these can happen inside here, so it should be removed.
| [GeneratedRegex(@"\d+$", RegexOptions.None)] | ||
| private static partial Regex PrNumberRegex(); | ||
|
|
||
| [GeneratedRegex(@"\d+$", RegexOptions.None)] | ||
| private static partial Regex IssueNumberRegex(); |
There was a problem hiding this comment.
One of them can go; you can use a broader name.
| return string.Empty; | ||
|
|
||
| // Capitalize first letter and ensure ends with period | ||
| var result = char.ToUpperInvariant(text[0]) + text.Substring(1); |
There was a problem hiding this comment.
Nit:
| var result = char.ToUpperInvariant(text[0]) + text.Substring(1); | |
| var result = text.Length < 2 | |
| ? char.ToUpperInvariant(text[0]).ToString() | |
| : char.ToUpperInvariant(text[0]) + text[1..]; |
| if (string.IsNullOrWhiteSpace(area)) | ||
| return string.Empty; | ||
|
|
||
| var result = char.ToUpperInvariant(area[0]) + area.Substring(1); |
There was a problem hiding this comment.
Nit:
| var result = char.ToUpperInvariant(area[0]) + area.Substring(1); | |
| var result = area.Length < 2 | |
| ? char.ToUpperInvariant(area[0]).ToString() | |
| : char.ToUpperInvariant(area[0]) + area[1..]; |
| var features = entriesByType.GetValueOrDefault("feature", []); | ||
| var enhancements = entriesByType.GetValueOrDefault("enhancement", []); | ||
| var security = entriesByType.GetValueOrDefault("security", []); | ||
| var bugFixes = entriesByType.GetValueOrDefault("bug-fix", []); | ||
|
|
||
| if (features.Count == 0 && enhancements.Count == 0 && security.Count == 0 && bugFixes.Count == 0) | ||
| { | ||
| // Still create file with "no changes" message | ||
| } | ||
|
|
||
| var hasBreakingChanges = entriesByType.ContainsKey("breaking-change"); | ||
| var hasDeprecations = entriesByType.ContainsKey("deprecation"); | ||
| var hasKnownIssues = entriesByType.ContainsKey("known-issue"); |
There was a problem hiding this comment.
Let's have a static class defining the type strings instead of using them directly here.
| if (input.InputProducts != null && input.InputProducts.Count > 0) | ||
| filterCount++; | ||
| if (input.Prs != null && input.Prs.Length > 0) | ||
| filterCount++; |
There was a problem hiding this comment.
Nit:
| if (input.InputProducts != null && input.InputProducts.Count > 0) | |
| filterCount++; | |
| if (input.Prs != null && input.Prs.Length > 0) | |
| filterCount++; | |
| if (input.InputProducts is { Count: > 0 }) | |
| filterCount++; | |
| if (input.Prs is { Length: > 0 }) | |
| filterCount++; |
| .Where(p => !string.IsNullOrWhiteSpace(p)) | ||
| .ToArray(); | ||
|
|
||
| if (input.Prs != null && input.Prs.Length > 0) |
There was a problem hiding this comment.
Nit:
| if (input.Prs != null && input.Prs.Length > 0) | |
| if (input.Prs is { Length: > 0 }) |
| _ = prsToMatch.Add(pr); | ||
| } | ||
| } | ||
| else if (input.Prs != null && input.Prs.Length > 0) |
There was a problem hiding this comment.
Nit:
| else if (input.Prs != null && input.Prs.Length > 0) | |
| else if (input.Prs is { Length: > 0 }) |
|
|
||
| // Build set of product/version combinations to filter by | ||
| var productsToMatch = new HashSet<(string product, string version)>(); | ||
| if (input.InputProducts != null && input.InputProducts.Count > 0) |
There was a problem hiding this comment.
Nit:
| if (input.InputProducts != null && input.InputProducts.Count > 0) | |
| if (input.InputProducts is { Count: > 0 }) |
|
|
||
| var data = deserializer.Deserialize<ChangelogData>(normalizedYaml); | ||
|
|
||
| if (data == null) |
There was a problem hiding this comment.
I think data can't be null at this point; so this if can be removed.
|
|
||
| // Set products array in output | ||
| // If --output-products was specified, use those values (override any from changelogs) | ||
| if (input.OutputProducts != null && input.OutputProducts.Count > 0) |
There was a problem hiding this comment.
Nit:
| if (input.OutputProducts != null && input.OutputProducts.Count > 0) | |
| if (input.OutputProducts is { Count: > 0 }) |
| return false; | ||
| } | ||
|
|
||
| if (data.Products == null || data.Products.Count == 0) |
There was a problem hiding this comment.
data.Products can't be null at this point.
| if (data.Products == null || data.Products.Count == 0) | |
| if (data.Products.Count == 0) |
| try | ||
| { | ||
| // Validate input | ||
| if (input.Bundles == null || input.Bundles.Count == 0) |
There was a problem hiding this comment.
Bundles can't be null at this point:
| if (input.Bundles == null || input.Bundles.Count == 0) | |
| if (input.Bundles.Count == 0) |
| if (bundledData == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Failed to deserialize bundle file"); | ||
| return false; | ||
| } | ||
|
|
||
| // Validate bundle has required structure | ||
| if (bundledData.Products == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Bundle file is missing required field: products"); | ||
| return false; | ||
| } | ||
|
|
||
| if (bundledData.Entries == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Bundle file is missing required field: entries"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
bundledData and these properties can't be null at this point. We need a distinction between actually missing the field (so it should be a nullable) and having it, with zero elements.
cotti
left a comment
cotti
left a comment
There was a problem hiding this comment.
I think this is mostly OK, but there's a question not quite solved:
Models like BundledChangelogData are always initializing their collection properties - but the logic asks if they are null when it seems like what's needed is for them to be just empty. So for these, can we guarantee if an empty collection is valid or not?
If it is, these checks need to change from == null to Any(). If it's not, then the model needs to change to make them a nullable reference, not initialized if there's no data for them in the input.
| if (bundledData == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Failed to deserialize bundle file"); | ||
| return false; | ||
| } | ||
|
|
||
| // Validate bundle has required structure | ||
| if (bundledData.Products == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Bundle file is missing required field: products"); | ||
| return false; | ||
| } | ||
|
|
||
| if (bundledData.Entries == null) | ||
| { | ||
| collector.EmitError(bundleInput.BundleFile, "Bundle file is missing required field: entries"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
bundledData and these properties can't be null at this point. We need a distinction between actually missing the field (so it should be a nullable) and having it, with zero elements.
And in this case, some of my nitpicks wouldn't apply: The current model is guaranteeing the creation of these collections in memory.
Added a
docs-builder changelog bundlecommand that bundles changelog fragments into a single YAML file.It's purpose is to create files like https://github.com/elastic/elasticsearch/blob/main/docs/release-notes/changelog-bundles/9.2.2.yml and https://github.com/elastic/elastic-agent/blob/main/changelog/9.2.2.yaml (which were created by the
gradlew bundleChangelogsandelastic-agent-changelog-tool buildcommands respectively).The command currently can create the list based on (a) all changelogs in a folder, (b) changelogs that have specific product and target values, (c) changelogs that have specific PR values. Only (a) was existing functionality. The long-term goal is to have these manifests generated from the list of PRs associated with a github release or deployment event (then optionally add known issues and security issues and remove feature-flagged changelogs as desired).
NOTE: #2352, #2408, and #2409 were also merged into this PR, which are stop-gap items (until publishing can be handled from a centralized site) to generate MD files from one or more changelog bundles (as is currently accomplished by the
gradlew generateReleaseNotesandelastic-agent-changelog-tool rendercommands in Elasticsearch and Elastic Agent respectively).Usage
Examples
Bundle a list of PRs
You can use the
--prsoption (with the--repoand--owneroptions if you provide only the PR numbers) to create a bundle of the changelogs that relate to those pull requests. For example:Bundle by PRs in file
If you specify the
--prs-fileoption, the bundle contains only changelogs that relate to those pull requests.For example, if you have a file with the following PR URLs:
Run the bundle command to reference this file and explicitly set the bundle's product metadata:
Alternatively, if the file contains just a list of PR numbers, you must specify the
--repoand--owneroptions:Both variations create a bundle like this:
In this example, none of the changelogs had
targetorlifecycleproduct values, therefore there's noversioninfo in this bundle.Bundle by product and target
If you specify the
--input-productsoption, the bundle contains only changelogs that contain one of the specified values:Even if the changelogs also have other product values, only those specified in the bundle command appear in the output:
Bundle all changelog files
NOTE: If you have changelogs that apply to multiple products and/or versions in your directory, this can result in a potentially unrealistic bundle. This command option was added to replicate existing behaviour in the
elastic-agent-changelog-tool buildandgradlew bundleChangelogcommands and will likely be deprecated.Copy the changelogs into the bundle
To include the contents of the changelogs, use the
--resolveoption:./docs-builder changelog bundle --prs 108875,135873,136886 --repo elasticsearch --owner elastic --output-products "elasticsearch 9.2.2" --resolveThis generates output that's similar to the existing Elastic Agent bundles, for example https://github.com/elastic/elastic-agent/blob/main/changelog/9.2.2.yaml
Features
BundledChangelogData.cs):BundledChangelogData- root structure with products and entriesBundledProduct- product/version pairsBundledEntry- individual changelog entries with all required fieldsBundledFile- file metadata with name and checksumChangelogService.BundleChangelogs):--all: Include all changelogs--input-products: Filter by product and version (e.g., "elastic-agent 9.1.5")--prs: Filter by PR URLs/numbers (supports multiple)--prs-file: Filter by PRs from a newline-delimited filehttps://github.com/owner/repo/pull/123owner/repo#123--ownerand--repooptionsChangelogCommand.Bundle):bundlesubcommand with all filtering options--outputto specify output file path--prsor--prs-file, the command:collector.EmitWarning()to report unmatched PRs--input-productsoption, the bundle filters changelog entries by thatproductandtargetvalue.--resolve, bundle entries only contain file with name and checksum--resolve, bundle entries include all changelog fields from the source files, with validationOutput format
The bundled YAML matches the requested format:
The command is ready to use. Build succeeds and the help text displays correctly.
Test coverage
Added 14 tests for the docs-builder changelog bundle command, covering:
BundleChangelogs_WithAllOption_CreatesValidBundle- Tests bundling all changelogs with--allBundleChangelogs_WithMultipleProducts_IncludesAllProducts- Tests handling multiple productsBundleChangelogs_WithProductVersionFilter_FiltersCorrectly- Tests--input-productsfilteringBundleChangelogs_WithPrsFilter_FiltersCorrectly- Tests--prsfilteringBundleChangelogs_WithPrsFileFilter_FiltersCorrectly- Tests--prs-filefilteringBundleChangelogs_WithPrNumberAndOwnerRepo_FiltersCorrectly- Tests PR number with--ownerand--repoBundleChangelogs_WithShortPrFormat_FiltersCorrectly- Tests short PR format (owner/repo#123)BundleChangelogs_WithPrsFilterAndUnmatchedPrs_EmitsWarnings- Verifies warnings for unmatched PRsBundleChangelogs_WithNoMatchingFiles_ReturnsError- No matching filesBundleChangelogs_WithInvalidDirectory_ReturnsError- Invalid directoryBundleChangelogs_WithNoFilterOption_ReturnsError- No filter option specifiedBundleChangelogs_WithMultipleFilterOptions_ReturnsError- Multiple filter optionsBundleChangelogs_WithInvalidProductVersionFormat_ReturnsError- Invalid product:version formatBundleChangelogs_WithInvalidPrsFile_ReturnsError- Invalid PRs file pathBundleChangelogs_WithResolve_CopiesChangelogContents- Verifies resolve copies all changelog fieldsBundleChangelogs_WithResolveAndMissingTitle_ReturnsError- Validates missing title failsBundleChangelogs_WithResolveAndMissingType_ReturnsError- Validates missing type failsBundleChangelogs_WithResolveAndMissingProducts_ReturnsError- Validates missing products failsBundleChangelogs_WithResolveAndInvalidProduct_ReturnsError- Validates invalid product entries failAll tests follow the same patterns as the existing
CreateChangelogtests:TestDiagnosticsCollectorto verify errors and warningsAll 14 tests pass.
Outstanding items
Generative AI disclosure
Tool(s) and model(s) used: composer-1 agent