Repository navigation
Support notes-product-version.json indices - #4120
Conversation
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4120/ Sample changed pages: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the current product/version notes flow can surface wrong-product notes in both CDN fallback and amend reconciliation paths.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
0457365 to
bf054a0
Compare
There was a problem hiding this comment.
No blocking issues found.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
cd4534a to
09940a0
Compare
Co-Authored-By: Claude <noreply@anthropic.com>
|
Hi @lcawl, you asked on Slack whether these note issues could be closed. The bugs in #4090 and #4103 are still there on main, so I'd rather finish this stack than drop it. I'm rebasing all three PRs onto current main and force-pushing your branches in a moment. Nothing in your code changed. Main moved 326 commits since 22 September, and the only conflicts were in tests:
Locally, the solution builds, Your phasing from #4103 still applies. Three green PRs are not three mergeable PRs: #4124 must wait until the docs-builder that reads Next: CI on all three, then a review. Martijn is back on 3 November. |
09940a0 to
846557a
Compare
![infra-vault-gh-plugin-prod[bot]](https://avatars.githubusercontent.com/u/6764390?s=60&v=4){kind=small}
| foreach (var (product, version) in noteTargets) | ||
| { | ||
| var noteEntries = await FetchCdnNotesAsync(collector, org, repo, noteTarget, ctx); | ||
| var noteEntries = await FetchCdnNotesAsync(collector, org, repo, product, version, ctx); |
There was a problem hiding this comment.
Severity: P3 (Low)
Add a bundling-service regression test with two output products at the same version, using distinct product-scoped indexes, and another case where both fall back to a shared legacy index. The added tests exercise the fetcher and reconciler independently, while existing bundle tests use one output product and legacy indexes. The new product/version iteration and cross-product checksum deduplication therefore have no service-level coverage; reverting to version-only target deduplication would still pass those tests.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| if (write.Product is not null) | ||
| return; | ||
| lock (written) | ||
| written[version] = sortedEntries; | ||
| written[write.Version!] = write.Entries; | ||
| }); |
There was a problem hiding this comment.
Severity: P2 (Medium)
The product-scoped indexes never receive the second-pass bundle_seq update. They are written with every sequence set to 0, but NoteAmendReconciler still rewrites only notes-{version}.json. Consequently, the new indexes permanently report already-shipped and amended notes as unreleased. Update the product-scoped indexes during amend reconciliation as well.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| { | ||
| var v = raw!.Trim(); | ||
| if (v.Contains('/', StringComparison.Ordinal)) | ||
| if (v.Contains('/', StringComparison.Ordinal) || !ChangelogKeys.IsValidRepo(v)) |
There was a problem hiding this comment.
Severity: P3 (Low)
Using IsValidRepo to validate a release version newly rejects valid SemVer build metadata, such as 9.3.0+build.1. Note creation accepts that version, and the previous reconciler indexed it, but this check now silently excludes the note from both the product index and the legacy version union, as well as amend reconciliation. Preserve the existing version support rather than applying repository-name character restrictions.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
Reviewed the source and tests. Additive as described: new notes-{product}-{version}.json keys, the legacy key still dual-written with no product/version in its body, and the fetcher falls back to the legacy key on a 404 only. A present-but-empty product index deliberately does not fall back. Product and version segments are validated before they become key parts. One minor note: version segments are now validated with IsValidRepo, which is stricter than the old /-only check, so a version with unusual characters is skipped with a warning instead of indexed. CI and Libra are green.
Summary
This PR adds product-scoped notes indexes that are additive.
To ensure we don't break existing changelog users, the legacy
notes-{version}.jsonkeys stay, and{changelog}still does not read those indexes.Background
Relates to #4090, #4103
Since the related issues touch the same areas of the code and have a significant, overlapping impact, this PR is part of a multi-PR phased approach.
Details
Indexes. The scrubber now writes
changelog/{org}/{repo}/notes-{product}-{version}.json(withproductandversionon the body) and still dual-writesnotes-{version}.jsonas a version union without those fields. Stale product-scoped keys can be deleted; a version-only key is kept while any note still declares that version.Consumers.
changelog bundleGETs the product-scoped URL first and falls back to the version-only URL only on HTTP 404. An empty product-scoped index does not fall back.MergeNotesAsyncwalks each(product, version)from--output-productsand still dedups by checksum.Docs (same change set).
docs/development/changelog-bundle-registry.md,docs/cli/changelog/cmd-note.md, anddocs/cli/changelog/cmd-backfill.md.The dual-write path is in the
changelog-scrubberLambda. It will not show up on the live CDN until that Lambda is deployed.Generative AI disclosure
Tool(s) and model(s) used: Cursor Grok 4.6