mirror of
https://github.com/coralogix/cx-cli.git
synced 2026-09-14 16:15:45 +08:00
Vitaly Lavrov
465742234a
## Context Adds `cx dashboards check`, a read-only subcommand that validates a dashboard definition against the Coralogix Dashboard Service's strict validator (`CheckDashboard`) without persisting it. This lets users and AI agents lint a dashboard JSON before `create`/`replace`, or re-validate an existing stored dashboard by id. Backed by [CX-47295](https://coralogix.atlassian.net/browse/CX-47295); the backend RPC already exists in eng-svc-dashboards-api (cx-api-dashboards v2.9.1). ## Linked Issues - Jira: [CX-47295](https://coralogix.atlassian.net/browse/CX-47295) — cx-cli: add `dashboards check` command to validate dashboard definitions via CheckDashboard - Related: CX-47080 (CheckDashboard wiring for cx-extensions — used as precedent for the proto/gateway REST path) ## Design Standard REST archetype (Archetype B), mirroring `DataArchiveMetricsCmd::Validate` as the only existing validate-style subcommand in the repo. - **Endpoint (confirmed from `cx-management-apis` proto):** `POST /mgmt/openapi/5/dashboards/check/v1`. The proto `google.api.http` annotation is `POST /dashboards/check/v1`; the management gateway maps the `/dashboards` prefix to `/mgmt/openapi/5/dashboards` (same mapping as the existing `DASHBOARDS_BASE`). Permission: `team-dashboards:Read` (read-only). - **Request oneof:** `CheckDashboardRequest` has a `source` oneof — `{ "dashboard": {...} }` for an inline definition or `{ "dashboardId": "<id>" }` for a stored dashboard. Reuses the existing `read_dashboard_body()` helper for the `--from-file` path (normalizes bare docs and `{ "dashboard": {...} }` wrappers, validates `layout` is present). - **Response:** `CheckDashboardResponse { issues: Vec<Issue> }` where `Issue { severity, message, location }`. `severity` is an enum (`SEVERITY_ERROR` / `SEVERITY_WARNING` / `SEVERITY_UNSPECIFIED`) serialized as SCREAMING_SNAKE_CASE per protobuf-JSON convention. `location` is an RFC 6901 JSON Pointer. Empty `issues` == valid. - **Three layers as per contributing/adding-a-command.md:** 1. `api.rs` — response types + `DashboardsApi::check()` + deserialization unit tests 2. `mod.rs` — `run_check()` with fan-out/merge/render + helpers 3. `main.rs` — `DashboardsCmd::Check` variant + dispatch arm - **Skill integration:** `cx-dashboards` skill updated with a new Phase 7 (server-side validation) inserted between Phase 6 (self-verify) and the deploy phase. Agents run `cx dashboards check --from-file <draft>` as a pre-flight gate; on issues, loop back to Phase 4 (regenerate JSON) → Phase 5 (re-verify queries) → Phase 7 again. ## Key Decisions - **Exit-code semantics: non-zero only on `SEVERITY_ERROR`.** The server today only emits `SEVERITY_ERROR`, but the proto reserves `SEVERITY_WARNING`. `SeverityUnspecified` is conservatively treated as a failure (it's the proto default and should never appear in real output). Warnings print but exit 0. Implemented via `IssueSeverity::is_failure()`. - **Multi-profile carve-out:** `check` exits non-zero if **any** profile returns error-severity issues, even if other profiles are clean. This deliberately breaks from the repo's "exit 0 if any profile succeeds" convention (`docs/multi-profile.md`) because `check` is a validation gate first. Per scope decision, the carve-out is documented **locally** (command `--help`, `run_check` doc comment, and the skill) rather than in `docs/multi-profile.md`, keeping the PR within domain-team + cxai review (no tech-writers needed). - **`from_file` has no default** (unlike `Create`/`Replace` which default to `-`). With a `-` default, `cx dashboards check` with no args would hang reading stdin; the explicit `Option<String>` + runtime guard produces a clear "specify either `--from-file` or a `<dashboard_id>`" error instead. - **Loop-back target on issues is Phase 4** (regenerate JSON), not Phase 5. Some strict issues are structural (missing variable/filter ids), not query-syntax — Phase 4 is the conservative default; after regenerating, Phase 5 re-verifies changed queries, then back to Phase 7. - **`--strict-check` flag on `create`/`replace` deferred.** Part 2 of the ticket (opt-in pre-flight flag on writes) was scoped out of this PR per explicit decision; the standalone `check` subcommand covers the lint-before-create use case on its own. ## Changes - **New subcommand:** `cx dashboards check` - `cx dashboards check --from-file <path|->` — validate an inline definition (reuses `read_dashboard_body` normalization; accepts bare doc or `{ "dashboard": {...} }` wrapper). - `cx dashboards check <dashboard-id>` — validate a stored dashboard by id. - Mutually exclusive sources via clap `conflicts_with` + runtime guard for the neither-supplied case. - Read-only — no `confirm_destructive`, not blocked by read-only mode. - **New output rendering for issues:** text (table `Severity | Location | Message` with colored severities; green `Dashboard is valid (no issues)` on empty), json (flat issue array with `profile` key when multi-profile), agents (TOON-encoded same shape). - **New API client method:** `DashboardsApi::check()` → `POST /mgmt/openapi/5/dashboards/check/v1`. - **Skill workflow change:** `cx-dashboards` skill phases renumbered — inserted Phase 7 (server-side validation), existing Phase 7 → 8 (Deploy), Phase 8 → 9 (Share link). Stale phase references in `references/deploy.md` and `references/verification.md` updated to match. ## Testing All run on this branch: - `cargo fmt --check` — clean - `cargo clippy --all-targets` — no warnings (fixed one `bool_comparison` in a unit test) - `cargo test --lib` — **404 passed**, 0 failed, 9 ignored (includes 5 new unit tests in `dashboards/api.rs`) - `cargo test --test dashboards` — **14 passed** (8 existing + 6 new wiremock integration tests) - `cargo build` — succeeds - `cx schema` — verified `check` present with `from_file` + `dashboard_id` args - `cx dashboards check --help` — renders correctly with examples + carve-out note - `cx dashboards --help` — `check` listed between `replace` and `delete` - `scripts/verify-skills.sh` — 14/14 passed - `cargo test --test e2e -- --ignored dashboards_check` — 2 passed (graceful skip without `CX_API_KEY`); not run against the real test team in this PR (CI workflow will run them) - Manual: `cx dashboards check` (no args) → clear error exit 1; `cx dashboards check --from-file foo <id>` (both args) → clap conflict error ## Risks & Rollout - **Backwards compatible.** New subcommand only; no changes to existing `catalog`/`get`/`create`/`replace`/`delete`/`search`/`query-search`/`folders` behavior. Default behavior of `create`/`replace` is unchanged (the deferred `--strict-check` flag would have been the only write-path change, and it's not in this PR). - **Endpoint path inferred from proto annotation.** `POST /mgmt/openapi/5/dashboards/check/v1` is derived from the proto `google.api.http` option + the established gateway prefix mapping. The e2e tests against the real test team will confirm the path resolves correctly; if it 404s, only the e2e tests fail (the subcommand is read-only and the integration tests mock the path, so there's no risk to existing flows). - **Skill workflow change is low-risk** — adds one checkpoint between self-verify and deploy; agents that skip it just go straight to deploy (no regression, just a missed pre-flight). - **Rollback:** revert the 7 commits; no migration, no config change, no data change. ## Out of Scope / Follow-ups - **`--strict-check` flag on `cx dashboards create`/`replace`** — Part 2 of CX-47295. Opt-in pre-flight validation before writes; off by default to preserve current behavior. Will be a separate PR. - **`--request-id` flag on `check** — the proto supports an optional idempotency key; not exposed in this PR. Can be added if needed. - **E2E against real test team** — the 2 `#[ignore]`d e2e tests skip gracefully without `CX_API_KEY`; CI's e2e workflow (`.github/workflows/e2e.yml`) will run them on merge to master. [CX-47295]: https://coralogix.atlassian.net/browse/CX-47295?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [CX-47295]: https://coralogix.atlassian.net/browse/CX-47295?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Yonatan Beker <yonbek@users.noreply.github.com>