Conversation
Adds an `allowedProducts` parameter to `ChangelogEntryValidator.Validate` and wires it up in `ChangelogEntryValidationService`. The set is built from the products that `products.yml` maps to the PR's repository via the `repository:` field. Entries that reference a known-but-disallowed product receive a clear error pointing contributors to the correct configuration. The `else if` structure ensures that a product that fails the global existence check does not also fire the per-repo restriction error. Co-Authored-By: Claude <noreply@anthropic.com>
Docs preview (local build)Handbook preview: https://docs-v3-preview.elastic.dev/elastic/docs-builder/pull/4116/ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes due to a functional allowlist regression in the new per-repo product restriction.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| knownProducts = new HashSet<string>(availableProducts.Keys.Select(k => k.Replace('_', '-')), StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| // ── Allowed products (per-repo restriction) ─────────────────────────────────────────── | ||
| var allowedProductIds = new HashSet<string>(matchedProducts.Select(p => p.Id.Replace('_', '-')), StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
[HIGH] allowedProductIds omits docset.yml release_notes products
This allowlist is built only from matchedProducts (repository-linked products in products.yml), but the validator contract and error message now state that products declared under release_notes: in docset.yml are also valid. As written, a repo that legitimately declares an additional release-notes product in docset.yml will be rejected as "not allowed for this repository".
Please build allowedProductIds from the union of repo-linked products and the resolved release_notes product IDs (using the same normalization).
Fetch docset.yml at the PR head ref and parse its release_notes: block to determine which product IDs are allowed for the submitting repo. Falls back to products.yml repository-field matching when docset.yml is absent or has no release_notes: section. Entries using a product outside that set now fail with a clear list of the allowed IDs. Co-Authored-By: Claude <noreply@anthropic.com>
Mpdreamz
changed the title
There was a problem hiding this comment.
Requesting changes: the new docset-based allowlist path currently fails to target the PR head ref, and the added validator tests still assert obsolete product-error text.
What is this? | From workflow: PR Review
Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.
| // Try both common locations for docset.yml. | ||
| foreach (var path in new[] { "docset.yml", ".config/docset.yml" }) | ||
| { | ||
| var url = @ref is not null ? $"{path}?ref={Uri.EscapeDataString(@ref)}" : path; |
There was a problem hiding this comment.
[HIGH] ?ref is encoded into the path, so docset lookup misses the PR head ref
FetchDocsetYamlAsync builds docset.yml?ref=... and passes it as path, but FetchFileContentAsync URL-escapes the entire path segment (/contents/{Uri.EscapeDataString(path)}). That turns ?ref= into %3Fref%3D..., so GitHub does not receive a query parameter and the request no longer targets the PR head ref.
Concrete failure: for a head ref like feat/changelog-restrict-entry-products, the request path becomes a lookup for docset.yml%3Fref=...; the call misses the intended revision and the logic falls back to matchedProducts, bypassing the docset-first restriction.
Please pass ref as a separate argument and append it as a real query string when building the API URL.
| knownProducts, | ||
| allowedProducts: allowedProducts | ||
| ); | ||
| findings.Should().ContainSingle(f => f.Message.Contains("not in the list of available products")); |
There was a problem hiding this comment.
[MEDIUM] New test expects obsolete validator message text
This new assertion still expects "not in the list of available products", but ChangelogEntryValidator now emits "is not a known product id" for known-product membership errors. As a result, the test suite fails on this branch.
A concrete repro is running dotnet test tests/Elastic.Changelog.Tests/, which fails this test (and Validate_UnknownProduct_ProducesError) on the message mismatch.
Please update these assertions to the current error contract (or assert on a stable error shape instead of legacy text).
1 participant
Changelog entry files are now rejected when they reference a product not declared for the submitting repository. The allowed set is resolved from
release_notes:in the repo'sdocset.ymlat the PR head ref; when that section is absent the validator falls back toconfig/products.ymlrepository-field matching. This catches copy-paste mistakes where a contributor from repo A accidentally tags a product owned by repo B.Affects: Changelog pipeline —
changelog validate-entriesgatePrompt summary: Restrict changelog entries to only reference products that are associated with the submitting repo — preferring the explicit
release_notes:declaration indocset.ymlover the implicit inference fromproducts.yml— so a repo cannot accidentally attribute changes to a product it does not own, and the error message lists only the repo's own products rather than the full 100+ product catalogue.Why
products.ymlalready maps each repository to its owned product(s), but the inference is approximate: a repo may publish multiple products, and itsproducts.ymlentries may differ from whatdocset.ymlexplicitly declares underrelease_notes:. Without this guard, any changelog entry in any repo can reference any product in the global catalogue. In practice, the playground repo's error listed all 100+ products, making the message useless, and a typo likeamp-agent-dotnetin a Kibana PR would pass silently.What
Docset-sourced product allowlist
ChangelogEntryValidationServicefetches the repo'sdocset.yml(or.config/docset.yml) at the PR head ref via a newIGitHubPrService.FetchFileContentAsyncmethod, then parses therelease_notes:block with a line-by-line YAML reader. Only product IDs that also appear inknownProducts(global products.yml set) survive the intersection — a typo indocset.ymldoes not bypass the global membership check. When norelease_notes:section is present, the service falls back to matching by therepository:field inproducts.yml.GitHub file-content fetcher
GitHubPrService.FetchFileContentAsynccalls the GitHub Contents API, base64-decodes the response, and returns the raw text. Returnsnullon 404 or any transport failure, so the allowlist logic degrades gracefully when the file does not exist.Per-repo allowlist in the validator
ChangelogEntryValidator.ValidatechecksallowedProductsbeforeknownProducts. When the allowlist is non-null, an out-of-set product produces"product 'X' is not allowed for this repository. Allowed products: <short list>"— not the 100+ product dump. The two branches are mutually exclusive: each product triggers at most one error.Tests
Four new tests cover: product in the allowed set (no error), product not in the set (error with short allowed list), null allowedProducts (falls through to known-products check), and mutual exclusion between the two error branches.
Verify
dotnet test tests/Elastic.Changelog.Tests/