Docs CLI: Add validation engine and validate command#2474
Conversation
|
Hello! 👋 This repository uses Auto for releasing packages using PR labels. ✨ This PR can be merged. It will not be considered when calculating future versions of the npm packages and will not appear in the changelogs. |
|
|
||
| const result = await validate({ docsPath }, allRules); | ||
|
|
||
| console.log(JSON.stringify(result, null, 2)); |
There was a problem hiding this comment.
More human readable format will come in upcoming PRs
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Grafana Catalog Team
sunker
added
the
no-changelog
There was a problem hiding this comment.
Pull request overview
Adds a validation foundation to @grafana/plugin-docs-cli by introducing a rule-based validation engine, initial filesystem rules, and a new validate CLI command. It also centralizes docsPath resolution in the CLI runner and updates existing commands to accept docsPath as an argument.
Changes:
- Introduce validation types, engine, and rule-category registry.
- Add initial filesystem validation rules with Vitest coverage.
- Add
validatecommand and refactorserve/buildto receive a resolveddocsPathfromrun.ts.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugin-docs-cli/src/validation/types.ts | Adds core validation types (Finding, Diagnostic, ValidationResult, etc.). |
| packages/plugin-docs-cli/src/validation/engine.ts | Implements validation engine that stamps severity via rule definitions. |
| packages/plugin-docs-cli/src/validation/engine.test.ts | Adds unit tests for engine behavior (severity stamping, async runners, etc.). |
| packages/plugin-docs-cli/src/validation/rules/index.ts | Registers rule categories (currently filesystem). |
| packages/plugin-docs-cli/src/validation/rules/filesystem.ts | Adds initial filesystem rules (root-index-exists, has-markdown-files). |
| packages/plugin-docs-cli/src/validation/rules/filesystem.test.ts | Adds tests for filesystem rules. |
| packages/plugin-docs-cli/src/commands/validate.command.ts | Adds validate command outputting JSON and setting exit code. |
| packages/plugin-docs-cli/src/commands/serve.command.ts | Refactors serve to accept docsPath from the runner. |
| packages/plugin-docs-cli/src/commands/build.command.ts | Refactors buildDocs to accept docsPath from the runner. |
| packages/plugin-docs-cli/src/commands/build.command.test.ts | Updates build tests to pass docsPath directly. |
| packages/plugin-docs-cli/src/bin/run.ts | Adds validate CLI wiring and centralizes docsPath resolution. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| /** | ||
| * All registered rule categories. | ||
| */ | ||
| export const allRules: RuleCategory[] = [{ definitions: filesystemDefinitions, run: checkFilesystem }]; |
There was a problem hiding this comment.
this system will quickly become boilerplately.
What if instead you let the reporter send the rule (just like the plugin-validator does) so your engine doesn't need to have this definitions variable around, just take it from the report itself.
Basically put RuleDefinition as part of Finding and let the validators submit it. it is faster and more explicit than later trying to figure out which reports belongs to which definition.
Additionally you can use enums so if someone for example starts sending a Finding that doesn't have a corresponding RuleDefinition the validator files to compile instead of failing to find the definitions
if you are worried about having different severities in strict and non-strict you can have a rule defining a Severity and StrictSeverity
There was a problem hiding this comment.
This is good feedback. Have made some changed: Rule runners now return Diagnostic[] directly with severity, so the definitions/lookup/assembly layer is gone entirely. Engine is just a loop now.
|
|
||
| if (!hasMarkdown) { | ||
| diagnostics.push({ | ||
| rule: 'has-markdown-files', |
There was a problem hiding this comment.
I really insist we should be using ENUMs here instead of magic strings
There was a problem hiding this comment.
Made rule name a const
| if (!hasMarkdown) { | ||
| diagnostics.push({ | ||
| rule: 'has-markdown-files', | ||
| severity: 'error', |
There was a problem hiding this comment.
likewise, severities must come from an enum (or enum const)
There was a problem hiding this comment.
Severity is a union type, see
github-project-automation
bot
moved this from 🔬 In review
to 🚀 Shipped
in Grafana Catalog Team
What this PR does / why we need it:
This PR adds a validation foundation to
@grafana/plugin-docs-cliwith a newvalidateCLI command and two basic filesystem rules (root-index-existsandhas-markdown-files).The validation architecture separates rules from the engine. Rule runners return
Finding[](rule ID, title, detail) without severity. The engine stamps severity fromRuleDefinitionmetadata and returns aValidationResultwithvalid: booleananddiagnostics: Diagnostic[]. The JSON output schema is designed to map directly to plugin-validator'sanalysis.Diagnostic.More filesystem rules and other rule categories (frontmatter, markdown, security, assets, cross-file) will follow in subsequent PRs. Output is JSON only for now - human-readable formatting comes later. For the full plan see the epic.
Which issue(s) this PR fixes:
Part of https://github.com/grafana/grafana-catalog-team/issues/769
Special notes for your reviewer:
wdocsPath resolution (reading src/plugin.json + checking the path exists) was centralized in run.ts instead of being duplicated across serve, build and validate. Each command now receives docsPath as a parameter.