diff --git a/docs/cli/release/changelog-bundle.md b/docs/cli/release/changelog-bundle.md index 379ae2b11..223800c4c 100644 --- a/docs/cli/release/changelog-bundle.md +++ b/docs/cli/release/changelog-bundle.md @@ -38,10 +38,10 @@ docs-builder changelog bundle [options...] [-h|--help] : Optional: The GitHub repository owner, which is required when pull requests are specified as numbers. `--prs ` -: Filter by pull request URLs or numbers (can specify multiple times). - -`--prs-file ` -: The path to a newline-delimited file containing PR URLs or numbers. +: Filter by pull request URLs or numbers (comma-separated), or a path to a newline-delimited file containing PR URLs or numbers. Can be specified multiple times. +: Each occurrence can be either comma-separated PRs (e.g., `--prs "https://github.com/owner/repo/pull/123,6789"`) or a file path (e.g., `--prs /path/to/file.txt`). +: When specifying PRs directly, provide comma-separated values. +: When specifying a file path, provide a single value that points to a newline-delimited file. `--repo ` : Optional: The GitHub repository name, which is required when PRs are specified as numbers. diff --git a/docs/contribute/changelog.md b/docs/contribute/changelog.md index bcb0b9e96..9d4cf2b4c 100644 --- a/docs/contribute/changelog.md +++ b/docs/contribute/changelog.md @@ -107,8 +107,7 @@ Options: --input-products ?> Filter by products in format "product target lifecycle, ..." (e.g., "cloud-serverless 2025-12-02, cloud-serverless 2025-12-06") [Default: null] --output-products ?> Explicitly set the products array in the output file in format "product target lifecycle, ...". Overrides any values from changelogs. [Default: null] --resolve Copy the contents of each changelog file into the entries array - --prs Filter by pull request URLs or numbers (can specify multiple times) [Default: null] - --prs-file Path to a newline-delimited file containing PR URLs or numbers [Default: null] + --prs Filter by pull request URLs or numbers (comma-separated), or a path to a newline-delimited file containing PR URLs or numbers. Can be specified multiple times. [Default: null] --owner Optional: GitHub repository owner (used when PRs are specified as numbers) [Default: null] --repo Optional: GitHub repository name (used when PRs are specified as numbers) [Default: null] ``` @@ -124,12 +123,11 @@ You can specify only one of the following filter options: : For example, `"cloud-serverless 2025-12-02, cloud-serverless 2025-12-06"`. `--prs` -: Include changelogs for the specified pull request URLs or numbers. -: Pull requests can be identified by a full URL (such as `https://github.com/owner/repo/pull/123`, a short format (such as `owner/repo#123`) or just a number (in which case you must also provide `--owner` and `--repo` options). - -`--prs-file` -: Include changelogs for the pull request URLs or numbers specified in a newline-delimited file. -: Pull requests can be identified by a full URL (such as `https://github.com/owner/repo/pull/123`, a short format (such as `owner/repo#123`) or just a number (in which case you must also provide `--owner` and `--repo` options). +: Include changelogs for the specified pull request URLs or numbers, or a path to a newline-delimited file containing PR URLs or numbers. Can be specified multiple times. +: Each occurrence can be either comma-separated PRs (e.g., `--prs "https://github.com/owner/repo/pull/123,12345"`) or a file path (e.g., `--prs /path/to/file.txt`). +: When specifying PRs directly, provide comma-separated values. +: When specifying a file path, provide a single value that points to a newline-delimited file. The file should contain one PR URL or number per line. +: Pull requests can be identified by a full URL (such as `https://github.com/owner/repo/pull/123`), a short format (such as `owner/repo#123`), or just a number (in which case you must also provide `--owner` and `--repo` options). By default, the output file contains only the changelog file names and checksums. You can optionally use the `--resolve` command option to pull all of the content from each changelog into the bundle. @@ -171,13 +169,13 @@ If you add the `--resolve` option, the contents of each changelog will be includ You can use the `--prs` option (with the `--repo` and `--owner` options if you provide only the PR numbers) to create a bundle of the changelogs that relate to those pull requests: ```sh -docs-builder changelog bundle --prs 108875,135873,136886 \ <1> +docs-builder changelog bundle --prs "108875,135873,136886" \ <1> --repo elasticsearch \ <2> --owner elastic \ <3> --output-products "elasticsearch 9.2.2" <4> ``` -1. The list of pull request numbers to seek. +1. The comma-separated list of pull request numbers to seek. You can also specify multiple `--prs` options, each with comma-separated PRs or a file path. 2. The repository in the pull request URLs. This option is not required if you specify the short or full PR URLs in the `--prs` option. 3. The owner in the pull request URLs. This option is not required if you specify the short or full PR URLs in the `--prs` option. 4. The product metadata for the bundle. If it is not provided, it will be derived from all the changelog product values. @@ -213,17 +211,20 @@ https://github.com/elastic/elasticsearch/pull/136886 https://github.com/elastic/elasticsearch/pull/137126 ``` -You can use the `--prs-file` option to create a bundle of the changelogs that relate to those pull requests: +You can use the `--prs` option with a file path to create a bundle of the changelogs that relate to those pull requests. You can also combine multiple `--prs` options: ```sh -./docs-builder changelog bundle --prs-file test/9.2.2.txt \ <1> +./docs-builder changelog bundle \ + --prs "https://github.com/elastic/elasticsearch/pull/108875,135873" \ <1> + --prs test/9.2.2.txt \ <2> --output-products "elasticsearch 9.2.2" <3> - --resolve <3> + --resolve <4> ``` -1. The path for the file that lists the pull requests. If the file contains only PR numbers, you must add `--repo` and `--owner` command options. -2. The product metadata for the bundle. If it is not provided, it will be derived from all the changelog product values. -3. Optionally include the contents of each changelog in the output file. +1. Comma-separated list of pull request URLs or numbers. +2. The path for the file that lists the pull requests. If the file contains only PR numbers, you must add `--repo` and `--owner` command options. +3. The product metadata for the bundle. If it is not provided, it will be derived from all the changelog product values. +4. Optionally include the contents of each changelog in the output file. If you have changelog files that reference those pull requests, the command creates a file like this: diff --git a/src/services/Elastic.Documentation.Services/Changelog/ChangelogBundleInput.cs b/src/services/Elastic.Documentation.Services/Changelog/ChangelogBundleInput.cs index 00c762f68..8a79bd5f5 100644 --- a/src/services/Elastic.Documentation.Services/Changelog/ChangelogBundleInput.cs +++ b/src/services/Elastic.Documentation.Services/Changelog/ChangelogBundleInput.cs @@ -16,7 +16,6 @@ public class ChangelogBundleInput public List? OutputProducts { get; set; } public bool Resolve { get; set; } public string[]? Prs { get; set; } - public string? PrsFile { get; set; } public string? Owner { get; set; } public string? Repo { get; set; } } diff --git a/src/services/Elastic.Documentation.Services/ChangelogService.cs b/src/services/Elastic.Documentation.Services/ChangelogService.cs index 845620090..c67f8b632 100644 --- a/src/services/Elastic.Documentation.Services/ChangelogService.cs +++ b/src/services/Elastic.Documentation.Services/ChangelogService.cs @@ -546,55 +546,145 @@ Cancel ctx filterCount++; if (input.Prs is { Length: > 0 }) filterCount++; - if (!string.IsNullOrWhiteSpace(input.PrsFile)) - filterCount++; if (filterCount == 0) { - collector.EmitError(string.Empty, "At least one filter option must be specified: --all, --input-products, --prs, or --prs-file"); + collector.EmitError(string.Empty, "At least one filter option must be specified: --all, --input-products, or --prs"); return false; } if (filterCount > 1) { - collector.EmitError(string.Empty, "Only one filter option can be specified at a time: --all, --input-products, --prs, or --prs-file"); + collector.EmitError(string.Empty, "Only one filter option can be specified at a time: --all, --input-products, or --prs"); return false; } - // Load PRs from file if specified + // Load PRs - check if --prs contains a file path or a list of PRs var prsToMatch = new HashSet(StringComparer.OrdinalIgnoreCase); - if (!string.IsNullOrWhiteSpace(input.PrsFile)) + if (input.Prs is { Length: > 0 }) { - if (!_fileSystem.File.Exists(input.PrsFile)) + // If there's exactly one value, check if it's a file path + if (input.Prs.Length == 1) { - collector.EmitError(input.PrsFile, "PRs file does not exist"); - return false; - } + var singleValue = input.Prs[0]; - var prsFileContent = await _fileSystem.File.ReadAllTextAsync(input.PrsFile, ctx); - var prsFromFile = prsFileContent - .Split('\n', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) - .Where(p => !string.IsNullOrWhiteSpace(p)) - .ToArray(); + // Check if it's a URL - URLs should always be treated as PRs, not file paths + var isUrl = singleValue.StartsWith("http://", StringComparison.OrdinalIgnoreCase) || + singleValue.StartsWith("https://", StringComparison.OrdinalIgnoreCase); - if (input.Prs is { Length: > 0 }) - { - foreach (var pr in input.Prs) + if (isUrl) { - _ = prsToMatch.Add(pr); + // Treat as PR identifier + _ = prsToMatch.Add(singleValue); } - } + else if (_fileSystem.File.Exists(singleValue)) + { + // File exists, read PRs from it + var prsFileContent = await _fileSystem.File.ReadAllTextAsync(singleValue, ctx); + var prsFromFile = prsFileContent + .Split('\n', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .Where(p => !string.IsNullOrWhiteSpace(p)) + .ToArray(); + + foreach (var pr in prsFromFile) + { + _ = prsToMatch.Add(pr); + } + } + else + { + // Check if it looks like a file path (contains path separators or has extension) + var looksLikeFilePath = singleValue.Contains(_fileSystem.Path.DirectorySeparatorChar) || + singleValue.Contains(_fileSystem.Path.AltDirectorySeparatorChar) || + _fileSystem.Path.HasExtension(singleValue); - foreach (var pr in prsFromFile) - { - _ = prsToMatch.Add(pr); + if (looksLikeFilePath) + { + // File path doesn't exist - if there are no other PRs, return error; otherwise emit warning + if (prsToMatch.Count == 0) + { + collector.EmitError(singleValue, $"File does not exist: {singleValue}"); + return false; + } + else + { + collector.EmitWarning(singleValue, $"File does not exist, skipping: {singleValue}"); + } + } + else + { + // Doesn't look like a file path, treat as PR identifier + _ = prsToMatch.Add(singleValue); + } + } } - } - else if (input.Prs is { Length: > 0 }) - { - foreach (var pr in input.Prs) + else { - _ = prsToMatch.Add(pr); + // Multiple values - process all values first, then check for errors + var nonExistentFiles = new List(); + foreach (var value in input.Prs) + { + // Check if it's a URL - URLs should always be treated as PRs + var isUrl = value.StartsWith("http://", StringComparison.OrdinalIgnoreCase) || + value.StartsWith("https://", StringComparison.OrdinalIgnoreCase); + + if (isUrl) + { + // Treat as PR identifier + _ = prsToMatch.Add(value); + } + else if (_fileSystem.File.Exists(value)) + { + // File exists, read PRs from it + var prsFileContent = await _fileSystem.File.ReadAllTextAsync(value, ctx); + var prsFromFile = prsFileContent + .Split('\n', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .Where(p => !string.IsNullOrWhiteSpace(p)) + .ToArray(); + + foreach (var pr in prsFromFile) + { + _ = prsToMatch.Add(pr); + } + } + else + { + // Check if it looks like a file path + var looksLikeFilePath = value.Contains(_fileSystem.Path.DirectorySeparatorChar) || + value.Contains(_fileSystem.Path.AltDirectorySeparatorChar) || + _fileSystem.Path.HasExtension(value); + + if (looksLikeFilePath) + { + // Track non-existent files to check later + nonExistentFiles.Add(value); + } + else + { + // Doesn't look like a file path, treat as PR identifier + _ = prsToMatch.Add(value); + } + } + } + + // After processing all values, handle non-existent files + if (nonExistentFiles.Count > 0) + { + // If there are no valid PRs and we have non-existent files, return error + if (prsToMatch.Count == 0) + { + collector.EmitError(nonExistentFiles[0], $"File does not exist: {nonExistentFiles[0]}"); + return false; + } + else + { + // Emit warnings for non-existent files since we have valid PRs + foreach (var file in nonExistentFiles) + { + collector.EmitWarning(file, $"File does not exist, skipping: {file}"); + } + } + } } } @@ -766,12 +856,6 @@ Cancel ctx } } - if (changelogEntries.Count == 0) - { - collector.EmitError(string.Empty, "No changelog entries matched the filter criteria"); - return false; - } - _logger.LogInformation("Found {Count} matching changelog entries", changelogEntries.Count); // Build bundled data @@ -805,7 +889,7 @@ Cancel ctx .ToList(); } // Otherwise, extract unique products/versions from changelog entries - else + else if (changelogEntries.Count > 0) { var productVersions = new HashSet<(string product, string version)>(); foreach (var (data, _, _, _) in changelogEntries) @@ -827,6 +911,18 @@ Cancel ctx }) .ToList(); } + else + { + // No entries and no products specified - initialize to empty list + bundledData.Products = []; + } + + // Check if we should allow empty result + if (changelogEntries.Count == 0) + { + collector.EmitError(string.Empty, "No changelog entries matched the filter criteria"); + return false; + } // Check for products with same product ID but different versions var productsByProductId = bundledData.Products.GroupBy(p => p.Product, StringComparer.OrdinalIgnoreCase) @@ -840,7 +936,12 @@ Cancel ctx } // Build entries - if (input.Resolve) + if (changelogEntries.Count == 0) + { + // No entries - initialize to empty list + bundledData.Entries = []; + } + else if (input.Resolve) { // When resolving, include changelog contents and validate required fields var resolvedEntries = new List(); diff --git a/src/tooling/docs-builder/Commands/ChangelogCommand.cs b/src/tooling/docs-builder/Commands/ChangelogCommand.cs index e1f2b5591..a20970970 100644 --- a/src/tooling/docs-builder/Commands/ChangelogCommand.cs +++ b/src/tooling/docs-builder/Commands/ChangelogCommand.cs @@ -110,8 +110,7 @@ async static (s, collector, state, ctx) => await s.CreateChangelog(collector, st /// Filter by products in format "product target lifecycle, ..." (e.g., "cloud-serverless 2025-12-02, cloud-serverless 2025-12-06") /// Explicitly set the products array in the output file in format "product target lifecycle, ...". Overrides any values from changelogs. /// Copy the contents of each changelog file into the entries array - /// Filter by pull request URLs or numbers (can specify multiple times) - /// Path to a newline-delimited file containing PR URLs or numbers + /// Filter by pull request URLs or numbers (comma-separated), or a path to a newline-delimited file containing PR URLs or numbers. Can be specified multiple times. /// Optional: GitHub repository owner (used when PRs are specified as numbers) /// Optional: GitHub repository name (used when PRs are specified as numbers) /// @@ -124,7 +123,6 @@ public async Task Bundle( [ProductInfoParser] List? outputProducts = null, bool resolve = false, string[]? prs = null, - string? prsFile = null, string? owner = null, string? repo = null, Cancel ctx = default @@ -134,6 +132,31 @@ public async Task Bundle( var service = new ChangelogService(logFactory, configurationContext, null); + // Process each --prs occurrence: each can be comma-separated PRs or a file path + var allPrs = new List(); + if (prs is { Length: > 0 }) + { + foreach (var prsValue in prs) + { + if (string.IsNullOrWhiteSpace(prsValue)) + continue; + + // Check if it contains commas - if so, split and add each as a PR + if (prsValue.Contains(',')) + { + var commaSeparatedPrs = prsValue + .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .Where(p => !string.IsNullOrWhiteSpace(p)); + allPrs.AddRange(commaSeparatedPrs); + } + else + { + // Single value - pass as-is (will be handled by service layer as file path or PR) + allPrs.Add(prsValue); + } + } + } + var input = new ChangelogBundleInput { Directory = directory ?? Directory.GetCurrentDirectory(), @@ -142,8 +165,7 @@ public async Task Bundle( InputProducts = inputProducts, OutputProducts = outputProducts, Resolve = resolve, - Prs = prs, - PrsFile = prsFile, + Prs = allPrs.Count > 0 ? allPrs.ToArray() : null, Owner = owner, Repo = repo }; diff --git a/tests/Elastic.Documentation.Services.Tests/ChangelogServiceTests.cs b/tests/Elastic.Documentation.Services.Tests/ChangelogServiceTests.cs index 660dc826c..2b0f22464 100644 --- a/tests/Elastic.Documentation.Services.Tests/ChangelogServiceTests.cs +++ b/tests/Elastic.Documentation.Services.Tests/ChangelogServiceTests.cs @@ -1087,7 +1087,7 @@ await fileSystem.File.WriteAllTextAsync(prsFile, """ var input = new ChangelogBundleInput { Directory = changelogDir, - PrsFile = prsFile, + Prs = new[] { prsFile }, Output = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "bundle.yaml") }; @@ -1342,7 +1342,7 @@ public async Task BundleChangelogs_WithMultipleProducts_CreatesValidBundle() } [Fact] - public async Task BundleChangelogs_WithInvalidPrsFile_ReturnsError() + public async Task BundleChangelogs_WithNonExistentFileAsPrs_ReturnsError() { // Arrange var service = new ChangelogService(_loggerFactory, _configurationContext, null); @@ -1350,10 +1350,12 @@ public async Task BundleChangelogs_WithInvalidPrsFile_ReturnsError() var changelogDir = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString()); fileSystem.Directory.CreateDirectory(changelogDir); + // Provide a non-existent file path - should return error since there are no other PRs + var nonexistentFile = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "nonexistent.txt"); var input = new ChangelogBundleInput { Directory = changelogDir, - PrsFile = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "nonexistent.txt"), + Prs = new[] { nonexistentFile }, Output = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "bundle.yaml") }; @@ -1361,9 +1363,98 @@ public async Task BundleChangelogs_WithInvalidPrsFile_ReturnsError() var result = await service.BundleChangelogs(_collector, input, TestContext.Current.CancellationToken); // Assert + // File doesn't exist and there are no other PRs, so should return error result.Should().BeFalse(); _collector.Errors.Should().BeGreaterThan(0); - _collector.Diagnostics.Should().Contain(d => d.Message.Contains("PRs file does not exist")); + _collector.Diagnostics.Should().Contain(d => d.Message.Contains("File does not exist")); + } + + [Fact] + public async Task BundleChangelogs_WithUrlAsPrs_TreatsAsPrIdentifier() + { + // Arrange + var service = new ChangelogService(_loggerFactory, _configurationContext, null); + var fileSystem = new FileSystem(); + var changelogDir = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString()); + fileSystem.Directory.CreateDirectory(changelogDir); + + // Create a changelog file for a specific PR + var changelog = """ + title: Test PR + type: feature + products: + - product: elasticsearch + target: 9.2.0 + pr: https://github.com/elastic/elasticsearch/pull/123 + """; + var changelogFile = fileSystem.Path.Combine(changelogDir, "1755268130-test-pr.yaml"); + await fileSystem.File.WriteAllTextAsync(changelogFile, changelog, TestContext.Current.CancellationToken); + + // Provide a URL - should be treated as a PR identifier, not a file path + var input = new ChangelogBundleInput + { + Directory = changelogDir, + Prs = new[] { "https://github.com/elastic/elasticsearch/pull/123" }, + Output = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "bundle.yaml") + }; + + // Act + var result = await service.BundleChangelogs(_collector, input, TestContext.Current.CancellationToken); + + // Assert + // URL should be treated as PR identifier and match the changelog + result.Should().BeTrue(); + _collector.Errors.Should().Be(0); + _collector.Warnings.Should().Be(0); + + var bundleContent = await fileSystem.File.ReadAllTextAsync(input.Output!, TestContext.Current.CancellationToken); + bundleContent.Should().Contain("name: 1755268130-test-pr.yaml"); + } + + [Fact] + public async Task BundleChangelogs_WithNonExistentFileAndOtherPrs_EmitsWarning() + { + // Arrange + var service = new ChangelogService(_loggerFactory, _configurationContext, null); + var fileSystem = new FileSystem(); + var changelogDir = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString()); + fileSystem.Directory.CreateDirectory(changelogDir); + + // Create a changelog file for a specific PR + var changelog = """ + title: Test PR + type: feature + products: + - product: elasticsearch + target: 9.2.0 + pr: https://github.com/elastic/elasticsearch/pull/123 + """; + var changelogFile = fileSystem.Path.Combine(changelogDir, "1755268130-test-pr.yaml"); + await fileSystem.File.WriteAllTextAsync(changelogFile, changelog, TestContext.Current.CancellationToken); + + // Provide a non-existent file path along with a valid PR - should emit warning for file but continue with PR + var nonexistentFile = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "nonexistent.txt"); + var input = new ChangelogBundleInput + { + Directory = changelogDir, + Prs = new[] { nonexistentFile, "https://github.com/elastic/elasticsearch/pull/123" }, + Output = fileSystem.Path.Combine(fileSystem.Path.GetTempPath(), Guid.NewGuid().ToString(), "bundle.yaml") + }; + + // Act + var result = await service.BundleChangelogs(_collector, input, TestContext.Current.CancellationToken); + + // Assert + // Should succeed because we have a valid PR, but should emit warning for the non-existent file + result.Should().BeTrue(); + _collector.Errors.Should().Be(0); + _collector.Warnings.Should().BeGreaterThan(0); + // Check that we have a warning about the file not existing + var fileWarning = _collector.Diagnostics.FirstOrDefault(d => d.Message.Contains("File does not exist, skipping")); + fileWarning.Should().NotBeNull("Expected a warning about the non-existent file being skipped"); + + var bundleContent = await fileSystem.File.ReadAllTextAsync(input.Output!, TestContext.Current.CancellationToken); + bundleContent.Should().Contain("name: 1755268130-test-pr.yaml"); } [Fact]