changelog: render CDN-hosted bundles in the {changelog} directive#3436
changelog: render CDN-hosted bundles in the {changelog} directive#3436cotti wants to merge 11 commits into
Conversation
Bundle uploads now refresh a per-product registry-index.json so consumers (the upcoming changelog directive cdn: mode) can enumerate bundles without an S3 listing. The builder does a read-merge-conditional-PUT (If-Match/If-None-Match) with bounded retries to stay safe under concurrent uploads, writes the manifest to the private bucket, and relies on the scrubber's existing pass-through to mirror it to the public CDN bucket. Refresh failures are non-fatal: bundle objects are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Lets the {changelog} directive load bundles published to the public
S3/CloudFront bucket via :cdn:, with optional :version: filtering, so a
docset can render another product's changelog without the bundle files
living in-repo. Also renames the per-product manifest from
registry-index.json to registry.json, reserving "registry-index" for a
possible future bucket-wide meta-registry.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 46 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements a complete end-to-end changelog bundle registry system enabling CDN-backed publishing and consumption. Producers upload bundles and trigger per-product 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: 4
🤖 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/CdnChangelogFetcher.cs`:
- Around line 65-67: The current code memoizes any result from FetchUncached
(var bundles = FetchUncached(...); _ = Cache.TryAdd(cacheKey, bundles);) which
can store empty or partial results; change this to only cache a clean, non-empty
fetch: update FetchUncached to return a result type (e.g., a struct/class or
tuple) that includes the bundles plus a success/hasWarnings flag (or throw on
registry failures), then in the caller check that result.Bundles is not empty
and result.HasWarnings is false (or result.Success is true) before calling
Cache.TryAdd(cacheKey, result.Bundles); otherwise skip caching to avoid
persisting transient CDN/404 failures.
In `@src/infra/docs-lambda-changelog-scrubber/README.md`:
- Around line 33-35: Update the README wording to clarify that the handler does
not copy arbitrary .json files: change the second bullet to state that only
registry manifests (keys matching RegistryKey.IsRegistry(...)) are passed
through by the handler; mention that other .json objects are skipped. Reference
the handler and the RegistryKey.IsRegistry(...) predicate to make the behavior
explicit.
In `@src/services/Elastic.Changelog/Uploading/RegistryBuilder.cs`:
- Around line 251-256: The catch-all that logs "Existing manifest ... could not
be parsed" is swallowing transient I/O/S3 errors and allowing a conditional
If-Match PUT (using the captured etag) to overwrite a valid manifest; change the
catch to only handle manifest parse-related exceptions (e.g., JsonException,
FormatException, InvalidDataException) or explicitly detect a parse failure, log
and return the rebuilt manifest as before, but rethrow or let other exceptions
(IOException, AmazonS3Exception/HttpRequestException, etc.) bubble up so
transient read errors are not treated as a corrupt manifest; locate the
try/catch around the response parsing in RegistryBuilder (the block that reads
response.ETag and parses the manifest, uses _logger.LogWarning and returns ([],
etag)) and update the exception handling accordingly.
In `@tests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs`:
- Around line 339-372: This test mutates the process-wide env var EnvVar via
Environment.SetEnvironmentVariable inside InitializeAsync(), causing flakiness;
instead either serialize the test or remove the global mutation by using an
injectable seam: change the test to call
CdnChangelogFetcher.PrimeCacheForTesting with a configurable base (add an
overload or setter on CdnChangelogFetcher to accept the CdnBase/Uri directly)
and stop setting Environment.SetEnvironmentVariable(EnvVar, CdnBase) (or restore
via a test-scoped lock); locate InitializeAsync(), the
Environment.SetEnvironmentVariable calls, and the PrimeCacheForTesting call and
update the test to pass the CDN base via the new injection point (or wrap the
env change in a process-wide test lock) so no global env var is mutated for
other tests.
🪄 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: 2c7ddd7d-5b9c-4326-b557-c37c8b7ac4b9
📒 Files selected for processing (22)
docs/development/changelog-bundle-registry.mddocs/development/toc.ymldocs/syntax/changelog.mdsrc/Elastic.Documentation.Configuration/Elastic.Documentation.Configuration.csprojsrc/Elastic.Documentation.Configuration/ReleaseNotes/BundleLoader.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/CdnChangelogFetcher.cssrc/Elastic.Documentation.Configuration/ReleaseNotes/ChangelogRegistry.cssrc/Elastic.Documentation/ReleaseNotes/ChangelogVersionMatch.cssrc/Elastic.Markdown/Myst/Directives/Changelog/ChangelogBlock.cssrc/Elastic.Markdown/Myst/Directives/DirectiveHtmlRenderer.cssrc/infra/docs-lambda-changelog-scrubber/Program.cssrc/infra/docs-lambda-changelog-scrubber/README.mdsrc/services/Elastic.Changelog/Uploading/ChangelogUploadService.cssrc/services/Elastic.Changelog/Uploading/Registry.cssrc/services/Elastic.Changelog/Uploading/RegistryBuilder.cssrc/services/Elastic.Changelog/Uploading/RegistryKey.cstests/Elastic.Changelog.Tests/Uploading/ChangelogUploadServiceTests.cstests/Elastic.Changelog.Tests/Uploading/RegistryBuilderTests.cstests/Elastic.Changelog.Tests/Uploading/RegistryKeyTests.cstests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/BundleLoaderFromContentTests.cstests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/CdnChangelogFetcherTests.cstests/Elastic.Markdown.Tests/Directives/ChangelogBasicTests.cs
Follows #3434, which moved bundle/changelog artifacts from {product}/bundles/ and {product}/changelogs/ to the singular {product}/bundle/ and {product}/changelog/. The CDN consumer fetched bundles from the old plural path, so it would 404 against the new layout; this points it at {product}/bundle/{file} and updates the registry docs/comments/tests to match. The directive's local-folder convention (changelog/bundles/) is unchanged. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PhysicalDocsetNavigationIncludesNestedTocs snapshots the real docs/development TOC. Adding changelog-bundle-registry.md introduced a fourth nav item (a file leaf), so the expected counts are bumped from 3 to 4 (and file leaves 0 to 1). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Quick ponder: Can that be optional? For most cases we can infer the product from the repository that holds the doc? |
Review fixes: - CdnChangelogFetcher only memoizes a clean, non-empty fetch so a transient CDN/replication failure no longer pins an empty/partial result for the process lifetime. - RegistryBuilder narrows the existing-manifest catch to JsonException, so a transient S3/IO read error bubbles to the best-effort refresh handler instead of letting an If-Match PUT overwrite a valid manifest with partial data. - Scrubber README clarifies only registry manifests (RegistryKey.IsRegistry) pass through; other .json keys are skipped. - CDN render test primes the fetcher cache against the default base instead of mutating the process-wide DOCS_BUILDER_CHANGELOG_CDN env var. Feature: -: cdn: product is now optional and inferred from the repository name when omitted (common 1:1 repo/product case); multi-product repos still specify it explicitly, and a clear error is emitted when it cannot be inferred. Co-authored-by: Cursor <cursoragent@cursor.com>
Ah, yes. Added inferring for the product as default. |
Import Elastic.Documentation.Configuration and use the unqualified Paths reference in the CDN inference test instead of the fully-qualified name. Co-authored-by: Cursor <cursoragent@cursor.com>
technige
left a comment
technige
left a comment
There was a problem hiding this comment.
Nice feature overall — the optimistic-concurrency design and the security guards are solid. A few things I'd suggest looking at before merging.
| Found = LoadedBundles.Count > 0; | ||
| } | ||
|
|
||
| private void ApplyLoadedBundles(BundleLoader loader, IReadOnlyList<LoadedBundle> loadedBundles) |
There was a problem hiding this comment.
Suggestion: loader is declared but never used inside ApplyLoadedBundles — looks like it was left over from the refactoring. Would it be okay to drop the parameter? The CDN call site on line 520 constructs a fresh BundleLoader just to satisfy the signature right now.
| return; | ||
| } | ||
|
|
||
| CdnProduct = product; |
There was a problem hiding this comment.
Suggestion: CdnProduct is assigned here, before LoadCdnBundles has had a chance to validate the product name. That means an invalid product like "invalid$product" ends up stored on the block even when an error is emitted (the test CapturesCdnProductOption documents this). Would it be cleaner to move IsValidCdnProduct check up here and only assign CdnProduct after it passes?
| private static readonly ConcurrentDictionary<string, IReadOnlyList<LoadedBundle>> Cache = new(StringComparer.Ordinal); | ||
|
|
||
| private readonly ILogger _logger = logFactory.CreateLogger<CdnChangelogFetcher>(); | ||
| private readonly HttpClient _httpClient = handler is null ? new HttpClient() : new HttpClient(handler, disposeHandler: false); |
There was a problem hiding this comment.
Suggestion: HttpClient is created per CdnChangelogFetcher instance but neither the fetcher nor ChangelogBlock implements IDisposable, so each instance leaks a socket handle. In practice the static cache means most fetcher instances never actually use _httpClient, but it's still a resource leak. Would it be worth either making CdnChangelogFetcher : IDisposable (and disposing after LoadCdnBundles returns), or sharing a static/injected HttpClient the way CrossLinkFetcher probably does?
| } | ||
|
|
||
| var fetcher = new CdnChangelogFetcher(NullLoggerFactory.Instance, Build.ReadFileSystem); | ||
| var loadedBundles = fetcher.Fetch(baseUri, product, VersionFilter, msg => this.EmitError(msg), msg => this.EmitWarning(msg), Cancel.None); |
There was a problem hiding this comment.
Nit: Cancel.None here means a build cancellation can't interrupt an in-flight CDN fetch. I understand this is a constraint of the synchronous Markdig pipeline — would a short comment be useful for the next reader so the choice doesn't look accidental?
| byFile[entry.File] = entry; | ||
|
|
||
| return byFile.Values | ||
| .OrderByDescending(b => b.Target ?? string.Empty, StringComparer.Ordinal) |
There was a problem hiding this comment.
Question: StringComparer.Ordinal sorts by string bytes, so "9.10.0" sorts before "9.9.0" (lexicographic). The docs say the manifest is "sorted newest first" — is ordinal close enough for the expected range of version strings, or would using the same VersionOrDate.Parse comparator the consumer uses be worth it here? (The consumer re-sorts anyway, so this only affects how registry.json looks on disk.)
| .MustHaveHappenedTwiceExactly(); | ||
|
|
||
| // The successful (second) PUT used the re-read ETag and merged both the concurrent and local entries. | ||
| _puts.Should().ContainSingle(); |
There was a problem hiding this comment.
Nit: _puts.Should().ContainSingle() here is checking the captured list, which only holds the PUT that didn't throw — the first (failed) call throws before _puts.Add runs. That's correct, but it's easy to misread as "only one PUT was attempted". A short comment like // first PUT threw, so only the successful retry is captured might save the next reader a double-take.
| - "999" | ||
| """) | ||
| ], _ => { }); | ||
| CdnChangelogFetcher.PrimeCacheForTesting(new Uri(ChangelogBlock.DefaultCdnBaseUrl), Product, null, bundles); |
There was a problem hiding this comment.
Nit: PrimeCacheForTesting takes new Uri(ChangelogBlock.DefaultCdnBaseUrl), which ties the test to the production constant being internal. That's a reasonable tradeoff given the synchronous-pipeline constraint, but would a brief comment here explaining why the production default URL has to match help future test authors avoid confusion?
| """) | ||
| { | ||
| [Fact] | ||
| public void CapturesCdnProductOption() => Block!.CdnProduct.Should().Be("invalid$product"); |
There was a problem hiding this comment.
Nit: This asserts CdnProduct == "invalid$product" even though the product fails validation. It accurately documents current behaviour, but it made me wonder whether CdnProduct should remain unset when the product is invalid (i.e., only assigned after IsValidCdnProduct passes). Leaving here as a question in case that's worth revisiting alongside the suggestion above.
|
I'm happy to test this out if a |
| ::: | ||
| ``` | ||
|
|
||
| The value names the product (must match `[a-zA-Z0-9_-]+`) and maps to `{product}/registry.json` plus the bundles it lists on the CDN. The value is **optional**: leave it blank to infer the product from the repository that holds the doc (the common case where the repository name is the product id, for example the `elasticsearch` repo renders the `elasticsearch` product). |
There was a problem hiding this comment.
The value names the product (must match
[a-zA-Z0-9_-]+)
My assumption is that it must also match a valid value from https://github.com/elastic/docs-builder/blob/main/config/products.yml Is that correct? If so, IMO that should be mentioned here too.
| ::: | ||
| ``` | ||
|
|
||
| Inference only works for repositories whose name matches the product id. Repositories that publish multiple products (for example `cloud`, which publishes `cloud-hosted`, `cloud-serverless`, and `cloud-enterprise`) must name the product explicitly; if a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles. |
There was a problem hiding this comment.
IMO we should use public repos in the example
| Inference only works for repositories whose name matches the product id. Repositories that publish multiple products (for example `cloud`, which publishes `cloud-hosted`, `cloud-serverless`, and `cloud-enterprise`) must name the product explicitly; if a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles. | |
| Inference only works for repositories whose name matches the product ID. For example, the `cloud-serverless` product release notes are published from the `docs-content` repo and therefore must specify the product explicitly. If a product cannot be inferred the block emits an error. When `:cdn:` is set, the local-folder argument is ignored. All other options (`:type:`, `:link-visibility:`, `:description-visibility:`, `:dropdowns:`, `:subsections:`) and `hide-features` apply identically to CDN-sourced bundles. |
- Validate :cdn: product before assigning CdnProduct so invalid names are never stored on the block (and update the test accordingly). - Make BundleLoader.MergeBundlesByTarget static and drop the throwaway BundleLoader the CDN path constructed just to satisfy the signature. - Use a process-shared static HttpClient (SocketsHttpHandler with PooledConnectionLifetime + a fetch timeout) in CdnChangelogFetcher to stop leaking a socket handle per directive; keep a per-instance client for injected test handlers and make the fetcher IDisposable. - Sort registry.json by VersionOrDate (not Ordinal) so the on-disk manifest is genuinely newest-first for mixed-width semvers; add a regression test. - Document the Cancel.None constraint, the single-PUT retry capture, and the cache-priming base URL. Co-authored-by: Cursor <cursoragent@cursor.com>
…o example
Address review feedback on the {changelog} :cdn: option docs:
- State that the product value is a product id defined in products.yml,
not just any [a-zA-Z0-9_-]+ string.
- Replace the multi-product `cloud` inference example with the public
`cloud-serverless`-published-from-`docs-content` example.
Co-authored-by: Cursor <cursoragent@cursor.com>
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
Design question — should CDN sourcing be declared in docset.yml?
The :cdn: option is per-directive inline configuration, meaning CDN dependencies are discovered at render time rather than at startup. This is the same concern raised on #3470 about changelog bundle's implicit CDN entry sourcing.
Compare how crosslinks work: repos are declared upfront in docset.yml under cross_links. docs-builder knows the external dependencies at startup, can fail fast if the index is unreachable, and fetches all link indexes concurrently before any page renders. The {changelog} directive gets none of that.
A docset-level declaration would look similar:
# docset.yml
release_notes:
- product: elasticsearch
- product: kibanaWith that:
- All registries and bundles are fetched once at startup, concurrently, before any page renders — rather than serially on first directive encounter.
- A CDN failure is a startup error with a clear config pointer, not a silent empty render mid-build.
- The
{changelog}directive just references an already-loaded in-memory set, with noHttpClientcalls at parse time. :cdn:could still exist as a convenience override, butdocset.ymlwould be the primary declaration path.
The process-wide ConcurrentDictionary cache means a product is only fetched once per build, so the per-directive overhead is bounded. But the fail-fast and concurrency gaps remain. Is this the direction you want, or is there a reason to keep it directive-inline?
Code issues
NullLoggerFactory.Instance for CDN fetches (ChangelogBlock.cs LoadCdnBundles): the fetcher is constructed with NullLoggerFactory.Instance, discarding all ILogger output (info-level fetch timing, debug-level cache hits). The block has access to build context — pass the real logger factory rather than null. CDN round-trip logs are exactly what you want when debugging a silent empty render.
repository == "unavailable" sentinel check (InferCdnProductFromRepository): this couples product inference to a specific sentinel string from the git information service. If that sentinel changes, inference silently fails with no diagnostic. A null/empty check or a typed signal from the git service would be safer.
Multi-line XML doc comments on private methods (LoadCdnBundles, FilterByVersion, ResolveCdnBaseUri, etc. in ChangelogBlock.cs): project style is one short line max on private methods, or none. Several new private methods have multi-sentence <summary> blocks.
Minor
ChangelogVersionMatch.Matches—null/empty → match-all: the non-nullablestring requestedparameter silently returnstruewhen empty/whitespace. A short comment explaining the match-all semantic would help callers.- Test gap: no test for
:cdn:+:version:together in CDN mode (both are tested independently but not combined).
Makes sense. I think adding it to docset also plays well with the intent of release notes being a product-centric entity in contrast with changelogs being a repo-level entity. The reasoning behind the initial approach was in allowing slighter quicker pacing, but with the benefit of hindsight we have consistently won more with tighter specs and RN already has a lot of input variety. I'll make the changes needed around, and notify the current products involved. |
4 participants
Why
Scrubbed changelog bundles live in the public S3 / CloudFront bucket, but the
{changelog}directive could previously only render bundles checked into the same repo. That meant a docset couldn't show another product's changelog without vendoring its bundle files, and there was no manifest to discover bundles without listing S3.What
registry.jsonmanifest in the public bucket (carried through by the scrubber's pass-through copy). Manifest writes use S3 conditional PUTs (If-Match/If-None-Match) with bounded retries so parallel uploads merge safely instead of clobbering each other.:cdn: <product>option on the{changelog}directive fetches that product'sregistry.jsonplus the referenced bundles from CloudFront and renders them inline. A new:version:option filters to a single version (matched against bundle target or filename); in CDN mode it only downloads the matching bundle.registry.json. "registry-index" is reserved for a possible future bucket-wide meta-registry, so the producer/consumer types and keys were renamed accordingly.How
CDN fetching happens inside the synchronous Markdig finalize pass, so
CdnChangelogFetcherdoes a synchronous HTTP fetch backed by a static in-memory cache (keyed by base URL + product + version) and the bundles it returns are self-contained. The base URL defaults to the production CloudFront domain and can be overridden viaDOCS_BUILDER_CHANGELOG_CDNfor testing.Made with Cursor